Page MenuHomePhabricator

Add -flookup-tables and -fno-lookup-tables flags
AbandonedPublic

Authored by sgundapa on Jul 18 2017, 12:56 PM.

Details

Summary

These flags control the lowering of switch statements to lookup tables.
When "-fno-lookup-tables" is passed, the function attribute "no-lookup-tables"
is set to "true"

Diff Detail

Event Timeline

sgundapa created this revision.Jul 18 2017, 12:56 PM
joerg added inline comments.Jul 18 2017, 3:28 PM
test/CodeGen/nouselookuptable.c
2

Check positive flag and the default case as well.

7

Just use f(void) or so. main is special enough, so avoiding it in test cases is often a good idea.

sgundapa updated this revision to Diff 107317.Jul 19 2017, 9:00 AM

Made the changes asked by reviewers

mcrosier added subscribers: echristo, ddunbar, mcrosier.

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

echristo edited edge metadata.Jul 20 2017, 6:39 PM

So, what's the overall logical idea behind the need for this option? While I understand that you don't want people just doing this via the -mllvm set of options, but if you're just doing this to provide a temporary option rather than turning it off in the backend I'm not sure what the point is.

Can you elaborate more on why you need to expose this via the front end?

The discussion is scattered across these patches https://reviews.llvm.org/D35578 and https://reviews.llvm.org/D35579.
I will provide a brief summary here:

The idea is to control the generation of data (lookup table) generated from a function, specifically when the user is not expecting it.
For hexagon, there is tightly coupled memory and the customers usually place "text" in it.
For functions, which generate lookup tables, it is very very expensive to read the table from a far away non-TCM data section.
This option will disable the generation of lookup tables at the expense of code bloat. This is really driven by the customers of hexagon backend.

This is not going to be a temporary option

The discussion is scattered across these patches https://reviews.llvm.org/D35578 and https://reviews.llvm.org/D35579.
I will provide a brief summary here:

The idea is to control the generation of data (lookup table) generated from a function, specifically when the user is not expecting it.
For hexagon, there is tightly coupled memory and the customers usually place "text" in it.
For functions, which generate lookup tables, it is very very expensive to read the table from a far away non-TCM data section.
This option will disable the generation of lookup tables at the expense of code bloat. This is really driven by the customers of hexagon backend.

I don't think we're communicating effectively. Let me try another way:

"Is there any reason why a user of the hexagon backend will ever not want to set these options to a particular value"

in other words:

"Should this just be part of the tuning for the hexagon backend and not options at all"

Thanks.

"Should this just be part of the tuning for the hexagon backend and not options at all"

This will be useful to all the backends/archs that support a tightly coupled memory.
AFAIK, hexagon is not the only target that has a TCM.

kparzysz edited edge metadata.Jul 24 2017, 8:09 AM

"Should this just be part of the tuning for the hexagon backend and not options at all"

The ultimate decision as to what kind of code to generate out of a switch statement belongs to the customer. The reasons to choose one way over another may involve factors that the compiler has no knowledge of (such as details of the target hardware). I don't think we need separate options to control jump tables and lookup tables: one option to enable/disable the use of memory tables would be sufficient.

If it is okay for the reviewers, I have no problem using -fno-jump-tables to this effect.
I need to update https://reviews.llvm.org/D35578 and https://reviews.llvm.org/D35579

"Should this just be part of the tuning for the hexagon backend and not options at all"

I don't think we need separate options to control jump tables and lookup tables: one option to enable/disable the use of memory tables would be sufficient.

I don't have an objection to this direction (i.e., disable the lookup-table optimization when -fno-jump-tables is specified), so long as other are in agreement.

I am waiting for others to approve/review the decision.

hans edited edge metadata.Jul 27 2017, 9:31 AM

"Should this just be part of the tuning for the hexagon backend and not options at all"

I don't think we need separate options to control jump tables and lookup tables: one option to enable/disable the use of memory tables would be sufficient.

I don't have an objection to this direction (i.e., disable the lookup-table optimization when -fno-jump-tables is specified), so long as other are in agreement.

I'm ok with disabling lookup tables under -fno-jump-tables as well.

Thanks. I will make change to this affect

The only change that is needed is to disable lookup-tables based on the attribute "no-jump-tables" set by "-fno-jump-tables" clang flag.
It implies that this change is not required.

sgundapa abandoned this revision.Jul 27 2017, 12:27 PM