Page MenuHomePhabricator

[SimplifyCFG] Make the no-jump-tables attribute also disable switch lookup tables
ClosedPublic

Authored by sgundapa on Jul 18 2017, 1:35 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

sgundapa created this revision.Jul 18 2017, 1:35 PM
joerg added inline comments.Jul 18 2017, 3:22 PM
lib/Transforms/Utils/SimplifyCFG.cpp
5155 ↗(On Diff #107168)

Just use Fn->getName() below directly.

5157 ↗(On Diff #107168)

...and the attribute "..." is not set.

test/Transforms/SimplifyCFG/Hexagon/disable-lookup-table.ll
60 ↗(On Diff #107168)

Can you reduce this further?

test/Transforms/SimplifyCFG/disable-lookup-table.ll
29 ↗(On Diff #107168)

Since this is a bit fragile when it comes to other optimizations deciding to merge the ranges, can you test the reverse as well? I.e. that it is turned into a lookup table when just the attribute is missing.

hans edited edge metadata.Jul 19 2017, 1:18 AM

What is the motivation for this?

For backends with "tightly coupled memory", in scenarios where the data is far away from text pays a good amount of penalty in terms of latency.
Hexagon is one such backend. The tables (both lookup and jump) which are being generated are treated as globals with internal linkage and by default
will be placed in read only data.

Interestingly when programmers specify the command line flag "-fno-jump-tables", they assume there is no data that goes in to other sections.
In case of llvm, the attribute "no-jump-tables" has no effect on simplifyCFG which generates the lookup table. This leads me to introduce "no-lookup-tables"

For backends with "tightly coupled memory", in scenarios where the data is far away from text pays a good amount of penalty in terms of latency.
Hexagon is one such backend. The tables (both lookup and jump) which are being generated are treated as globals with internal linkage and by default
will be placed in read only data.

Interestingly when programmers specify the command line flag "-fno-jump-tables", they assume there is no data that goes in to other sections.
In case of llvm, the attribute "no-jump-tables" has no effect on simplifyCFG which generates the lookup table. This leads me to introduce "no-lookup-tables"

It looks like TargetTransformInfo already has a hook, shouldBuildLookupTables(), for disabling this optimization. Why not just set that to false for Hexagon?

sgundapa updated this revision to Diff 107324.Jul 19 2017, 9:41 AM

Made changes as per reviewers comments

sgundapa added inline comments.Jul 19 2017, 9:43 AM
lib/Transforms/Utils/SimplifyCFG.cpp
5155 ↗(On Diff #107168)

FuncName is being used below to set the name of the table to switch.table.<FuncName>

It looks like TargetTransformInfo already has a hook, shouldBuildLookupTables(), for disabling this optimization. Why not just set that to false for Hexagon?

TCM is often small and you don't want to either enable or disable the table generation for a specific backend. As of today,
hexagon-emit-lookup-tables llvm flag control the generation of lookup tables for hexagon backend.
The real problem here is , I do not want the user to use a combination of front end and backend flags to get the desired result.

mcrosier added subscribers: echristo, ddunbar.

Adding @echristo and @ddunbar who have been the primary owners of the driver for the past decade or so.

sgundapa updated this revision to Diff 108516.Jul 27 2017, 12:49 PM
sgundapa edited the summary of this revision. (Show Details)

We have decided to have this feature under "-fno-jump-tables" clang flag.
To this affect, this patch updates the code to control the generation of
lookup tables based on the attribute "no-jump-tables"

hans added a comment.Jul 27 2017, 6:30 PM

"Generate lookup tables based on a function attribute" is not very descriptive. How about "Make the no-jump-tables attribute also disable switch lookup tables"?

Also it makes me sad that the attribute isn't documented anywhere. Would you like to look into that, possible as a follow-up patch?

lib/Transforms/Utils/SimplifyCFG.cpp
5155 ↗(On Diff #108516)

There doesn't seem to be any need to move FuncName up here anymore, since it's not used until further down.

sgundapa updated this revision to Diff 108652.Jul 28 2017, 7:17 AM
sgundapa retitled this revision from [SimplifyCFG] Generate lookup tables based on a function attribute to [SimplifyCFG] Make the no-jump-tables attribute also disable switch lookup tables.
sgundapa edited the summary of this revision. (Show Details)

Also it makes me sad that the attribute isn't documented anywhere. Would you like to look into that, possible as a follow-up patch?

Sure. I will push a follow up patch

hans accepted this revision.Jul 28 2017, 2:12 PM
This revision is now accepted and ready to land.Jul 28 2017, 2:12 PM
mcrosier accepted this revision.Jul 28 2017, 2:13 PM
This revision was automatically updated to reflect the committed changes.

I moved SimplifyCFG/disable-lookup-table.ll into SimplifyCFG/X86/disable-lookup-table.ll to avoid test failure when X86 backend is not enabled (rL309487).
Please update again if you prefer another way to avoid test failure. Thanks!