Page MenuHomePhabricator

Add -fno-jump-tables and-fjump-tables flags
ClosedPublic

Authored by niravd on Mar 23 2016, 10:47 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

niravd updated this revision to Diff 51444.Mar 23 2016, 10:47 AM
niravd retitled this revision from to Add -fno-jump-tables and-fjump-tables flags.
niravd added reviewers: echristo, hans.
niravd updated this object.
niravd added a subscriber: llvm-commits.
echristo edited edge metadata.Mar 23 2016, 3:03 PM

Hrm. So it can be changed in the future do we want it to be a string attribute similar to use-soft-float rather than a hard coded one? (I think the answer is "probably")

-eric

niravd updated this revision to Diff 51790.Mar 28 2016, 7:28 AM
niravd edited edge metadata.

Modification to use soft attrbiutes as suggested

hans edited edge metadata.Mar 28 2016, 11:00 AM

Besides a few nits, this looks good as far as I can tell. Eric should probably sign off on it, though.

include/clang/Driver/Options.td
590 ↗(On Diff #51790)

It's not just the size of the switch, but also the density, etc.

I think it would make more sense to add HelpText to the -fno flag below instead. How about:

"Do not use jump tables for lowering switches"

lib/CodeGen/CodeGenFunction.cpp
715 ↗(On Diff #51790)

nit: period at the end of comment, and 80+ column line

test/CodeGen/nousejumptable.c
3 ↗(On Diff #51790)

ultra nit: there's usually a space between the // and CHECK

6 ↗(On Diff #51790)

Is there a need for volatile here? Otherwise, drop it.

niravd updated this revision to Diff 51915.Mar 29 2016, 8:04 AM
niravd marked 4 inline comments as done.
niravd edited edge metadata.

Change help message and update nojumptable test for new string attribute

include/clang/Driver/Options.td
590 ↗(On Diff #51790)

This is help message for from GCC, but I agree this is clearer

mehdi_amini added inline comments.
test/CodeGen/nousejumptable.c
43 ↗(On Diff #51915)

The only thing this test does is checking for the attribute. All the code is useless. You can have a single function

define void @foo() {
   ret void
}

And it should be enough to check that the attribute is added

niravd updated this revision to Diff 51925.Mar 29 2016, 8:35 AM
niravd marked an inline comment as done.

Simplify test

I'm waiting for an official okay from someone on this.

hans accepted this revision.Apr 5 2016, 10:30 AM
hans edited edge metadata.

LGTM

include/clang/Frontend/CodeGenOptions.def
152 ↗(On Diff #51925)

ultra nit: period at end of comment

This revision is now accepted and ready to land.Apr 5 2016, 10:30 AM
This revision was automatically updated to reflect the committed changes.