[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Sumanth Gundapaneni via Phabricator via cfe-commits]

sgundapa added a comment.

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.


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Sumanth Gundapaneni via Phabricator via cfe-commits]

sgundapa added a comment.

Thanks. I will make change to this affect


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

In https://reviews.llvm.org/D35577#818956, @mcrosier wrote:

> In https://reviews.llvm.org/D35577#818936, @kparzysz wrote:
>
> > In https://reviews.llvm.org/D35577#817944, @echristo wrote:
> >
> > > "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.


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Sumanth Gundapaneni via Phabricator via cfe-commits]

sgundapa added a comment.

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


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Chad Rosier via Phabricator via cfe-commits]

mcrosier added a comment.

In https://reviews.llvm.org/D35577#818936, @kparzysz wrote:

> In https://reviews.llvm.org/D35577#817944, @echristo wrote:
>
> > "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.


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Sumanth Gundapaneni via Phabricator via cfe-commits]

sgundapa added a comment.

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


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Krzysztof Parzyszek via Phabricator via cfe-commits]

kparzysz added a comment.

In https://reviews.llvm.org/D35577#817944, @echristo wrote:

> "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.


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Sumanth Gundapaneni via Phabricator via cfe-commits]

sgundapa added a comment.

> "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.


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Eric Christopher via Phabricator via cfe-commits]

echristo added a comment.

In https://reviews.llvm.org/D35577#817267, @sgundapa wrote:

> 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.


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Sumanth Gundapaneni via Phabricator via cfe-commits]

sgundapa added a comment.

This is not going to be a temporary option


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Sumanth Gundapaneni via Phabricator via cfe-commits]

sgundapa added a comment.

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.


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Eric Christopher via Phabricator via cfe-commits]

echristo added a comment.

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?


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Chad Rosier via Phabricator via cfe-commits]

mcrosier added subscribers: echristo, ddunbar, mcrosier.
mcrosier added reviewers: echristo, ddunbar.
mcrosier added a comment.

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


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Sumanth Gundapaneni via Phabricator via cfe-commits]

sgundapa updated this revision to Diff 107317.
sgundapa added a comment.

Made the changes asked by reviewers


https://reviews.llvm.org/D35577

Files:
  include/clang/Driver/Options.td
  include/clang/Frontend/CodeGenOptions.def
  lib/CodeGen/CodeGenFunction.cpp
  lib/Driver/ToolChains/Clang.cpp
  lib/Frontend/CompilerInvocation.cpp
  test/CodeGen/nouselookuptable.c


Index: test/CodeGen/nouselookuptable.c
===
--- /dev/null
+++ test/CodeGen/nouselookuptable.c
@@ -0,0 +1,14 @@
+// RUN: %clang -S -fno-lookup-tables %s -emit-llvm -o - \
+// RUN: | FileCheck --check-prefix=NOLOOKUP %s
+// NOLOOKUP: @foo
+// NOLOOKUP: attributes #0 = {{.*}}"no-lookup-tables"="true"{{.*}}
+
+// RUN: %clang -S %s -emit-llvm -o - | FileCheck --check-prefix=LOOKUP %s
+// RUN: %clang -S -flookup-tables %s -emit-llvm -o - \
+// RUN: | FileCheck --check-prefix=LOOKUP %s
+// LOOKUP: @foo
+// LOOKUP: attributes #0 = {{.*}}"no-lookup-tables"="false"{{.*}}
+
+void foo() {
+  return;
+}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp
+++ lib/Frontend/CompilerInvocation.cpp
@@ -652,6 +652,8 @@
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
 
+  Opts.NoUseLookupTables = Args.hasArg(OPT_fno_lookup_tables);
+
   Opts.PrepareForLTO = Args.hasArg(OPT_flto, OPT_flto_EQ);
   Opts.EmitSummaryIndex = false;
   if (Arg *A = Args.getLastArg(OPT_flto_EQ)) {
Index: lib/Driver/ToolChains/Clang.cpp
===
--- lib/Driver/ToolChains/Clang.cpp
+++ lib/Driver/ToolChains/Clang.cpp
@@ -2266,6 +2266,10 @@
 true))
 CmdArgs.push_back("-fno-jump-tables");
 
+  if (!Args.hasFlag(options::OPT_flookup_tables, 
options::OPT_fno_lookup_tables,
+true))
+CmdArgs.push_back("-fno-lookup-tables");
+
   if (!Args.hasFlag(options::OPT_fpreserve_as_comments,
 options::OPT_fno_preserve_as_comments, true))
 CmdArgs.push_back("-fno-preserve-as-comments");
Index: lib/CodeGen/CodeGenFunction.cpp
===
--- lib/CodeGen/CodeGenFunction.cpp
+++ lib/CodeGen/CodeGenFunction.cpp
@@ -806,6 +806,10 @@
   Fn->addFnAttr("no-jump-tables",
 llvm::toStringRef(CGM.getCodeGenOpts().NoUseJumpTables));
 
+  // Add no-lookup-tables value.
+  Fn->addFnAttr("no-lookup-tables",
+llvm::toStringRef(CGM.getCodeGenOpts().NoUseLookupTables));
+
   if (getLangOpts().OpenCL) {
 // Add metadata for a kernel function.
 if (const FunctionDecl *FD = dyn_cast_or_null(D))
Index: include/clang/Frontend/CodeGenOptions.def
===
--- include/clang/Frontend/CodeGenOptions.def
+++ include/clang/Frontend/CodeGenOptions.def
@@ -177,6 +177,7 @@
 CODEGENOPT(UnrollLoops   , 1, 0) ///< Control whether loops are unrolled.
 CODEGENOPT(RerollLoops   , 1, 0) ///< Control whether loops are rerolled.
 CODEGENOPT(NoUseJumpTables   , 1, 0) ///< Set when -fno-jump-tables is enabled.
+CODEGENOPT(NoUseLookupTables , 1, 0) ///< Set when -fno-lookup-tables is 
enabled.
 CODEGENOPT(UnsafeFPMath  , 1, 0) ///< Allow unsafe floating point optzns.
 CODEGENOPT(UnwindTables  , 1, 0) ///< Emit unwind tables.
 CODEGENOPT(VectorizeLoop , 1, 0) ///< Run loop vectorizer.
Index: include/clang/Driver/Options.td
===
--- include/clang/Driver/Options.td
+++ include/clang/Driver/Options.td
@@ -792,6 +792,9 @@
 def fjump_tables : Flag<["-"], "fjump-tables">, Group;
 def fno_jump_tables : Flag<["-"], "fno-jump-tables">, Group, 
Flags<[CC1Option]>,
   HelpText<"Do not use jump tables for lowering switches">;
+def flookup_tables : Flag<["-"], "flookup-tables">, Group;
+def fno_lookup_tables : Flag<["-"], "fno-lookup-tables">, Group, 
Flags<[CC1Option]>,
+  HelpText<"Do not use lookup tables for lowering switches">;
 
 // Begin sanitizer flags. These should all be core options exposed in all 
driver
 // modes.


Index: test/CodeGen/nouselookuptable.c
===
--- /dev/null
+++ test/CodeGen/nouselookuptable.c
@@ -0,0 +1,14 @@
+// RUN: %clang -S -fno-lookup-tables %s -emit-llvm -o - \
+// RUN: | FileCheck --check-prefix=NOLOOKUP %s
+// NOLOOKUP: @foo
+// NOLOOKUP: attributes #0 = {{.*}}"no-lookup-tables"="true"{{.*}}
+
+// RUN: %clang -S %s -emit-llvm -o - | FileCheck --check-prefix=LOOKUP %s
+// RUN: %clang -S -flookup-tables %s -emit-llvm -o - \
+// RUN: | FileCheck --check-prefix=LOOKUP %s
+// LOOKUP: @foo
+// LOOKUP: attributes #0 = {{.*}}"no-lookup-tables"="false"{{.*}}
+
+void foo() {
+  return;
+}
Index: lib/Frontend/CompilerInvocation.cpp
===
--- lib/Frontend/CompilerInvocation.cpp

[PATCH] D35577: Add -flookup-tables and -fno-lookup-tables flags

[Joerg Sonnenberger via Phabricator via cfe-commits]

joerg added inline comments.



Comment at: test/CodeGen/nouselookuptable.c:1
+// RUN: %clang_cc1 -S -fno-lookup-tables %s -emit-llvm -o - | FileCheck %s
+

Check positive flag and the default case as well.



Comment at: test/CodeGen/nouselookuptable.c:6
+
+int main() {
+  return 0;

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


https://reviews.llvm.org/D35577



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits