Page MenuHomePhabricator

Add support for no-jump-tables
ClosedPublic

Authored by niravd on Mar 21 2016, 10:56 AM.

Details

Summary

Add function attribute to represent the avoidance of generation of jump tables.
This is an initial step to supporting gcc's -fno-jump-tables flag.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd updated this revision to Diff 51189.Mar 21 2016, 10:56 AM
niravd retitled this revision from to Add support for no-jump-tables flag..
niravd updated this object.
niravd added a reviewer: hans.
niravd added a subscriber: llvm-commits.
hans added inline comments.Mar 22 2016, 8:59 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7930 ↗(On Diff #51189)

There is already a helper function in this file that decides whether or not to build jump tables: areJTsAllowed(). That's a better place to do this check.

If we really want to disable jump tables, maybe the proper thing to do is to make TLI.isOperationLegalOrCustom(BR_JT) return false. I don't know if that's more or less convenient to do, though.

niravd updated this revision to Diff 51436.Mar 23 2016, 9:40 AM

Rebased no-jump-tables to key on Function Attributes not target options and removed
added llc command line flag as we can now pass this information textually in the file

niravd retitled this revision from Add support for no-jump-tables flag. to Add support for no-jump-tables.Mar 23 2016, 9:43 AM
niravd updated this object.

Code look straightforward. Using a function attribute looks good to me. See a nit below.

test/CodeGen/X86/switch.ll
46 ↗(On Diff #51436)

Update comment: Should never be lowered as a jump table because of the attribute.

niravd marked an inline comment as done.Mar 23 2016, 11:00 AM

Comment added to switch.ll

hans edited edge metadata.Mar 23 2016, 11:43 AM

I missed where someone suggested making this a function attribute.

What's the motivation for wanting some functions to get jump tables, and not others? Isn't it something you'd want for the whole program?

What happens if a function with nousejumptable gets inlined into a function without the attribute?

echristo edited edge metadata.Mar 23 2016, 3:01 PM

So having it as a function attribute was my idea :)

Otherwise, it's a good question. What should happen in LTO when you ask for one translation unit to be compiled without jump table support and the other one to be compiled with jump table support? Which one wins here? For some things it's more obvious, I'm not sure here.

-eric

hans added a comment.Mar 23 2016, 3:10 PM

So having it as a function attribute was my idea :)

Otherwise, it's a good question. What should happen in LTO when you ask for one translation unit to be compiled without jump table support and the other one to be compiled with jump table support? Which one wins here? For some things it's more obvious, I'm not sure here.

I suppose it depends on the motivation for this patch. Why would one be compiling with -no-jump-tables? Is it just an optimization thing, or could there be a situation where a jump table would actually not work, despite the target supporting it in general? (Apologies if this is obvious, but it would be great to have it explained in the description of this patch.)

I don't know either so I'm somewhat curious myself :)

-eric

The GCC doc for the option is:

OK, sounds like it should poison on inlining then. (And likely is going to be used everywhere, but maximum safety here).

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

Make no-jump-tables soft attribute as suggested in D18407 and union merged no-jump-tables flags on inline

niravd added inline comments.Mar 28 2016, 7:22 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7930 ↗(On Diff #51189)

Realized I hadn't addressed this. This was probably the right thing originally, but isn't possible now that no-jump-tables is a function attribute.

hans added inline comments.Mar 28 2016, 10:51 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7957 ↗(On Diff #51787)

Please make areJTsAllowed() take a Function* parameter and do the check there instead. That way, the code will not waste time in findJumpTables() only to decide later to not build any.

hans added a comment.Mar 28 2016, 11:01 AM

Please make areJTsAllowed() take a Function* parameter and do the check there instead. That way, the code will not waste time in findJumpTables() only to decide later to not build any.

niravd updated this revision to Diff 51913.Mar 29 2016, 7:58 AM
niravd marked an inline comment as done.

Move no-jump-tables code into areJTsAllowed

niravd updated this revision to Diff 51914.Mar 29 2016, 8:01 AM

Apply clang-format

mehdi_amini added inline comments.Mar 29 2016, 8:21 AM
test/Transforms/Inline/attributes.ll
246 ↗(On Diff #51914)

Why are you killing the noimplicitfloat test?

niravd added inline comments.Mar 29 2016, 8:26 AM
test/Transforms/Inline/attributes.ll
246 ↗(On Diff #51914)

That's a good question. This was copied over at one point. I must have botched a rebase. Will fix.

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

Fix test.

hans added inline comments.Mar 29 2016, 8:48 AM
lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7961 ↗(On Diff #51924)

I don't think the comment adds value here; it's very clear that this is what the code below does.

7962 ↗(On Diff #51924)

The variable should be named Fn.

7964 ↗(On Diff #51924)

Would just

if (Fn->getFnAttribute("no-jump-tables").getValueAsString() == "true")

work?

niravd updated this revision to Diff 51942.Mar 29 2016, 9:55 AM
niravd marked 2 inline comments as done.

Cleanup FnAttr lookup

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
7964 ↗(On Diff #51924)

It does. There are a few more instances of this pattern in the codebase. I'll submit those separately.

hans added a comment.Mar 29 2016, 9:58 AM

Thanks! This looks good to me.

This revision was automatically updated to reflect the committed changes.