[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via cfe-commits]

gulfem marked 17 inline comments as done.
gulfem added inline comments.


================
Comment at: clang/test/CodeGen/switch-to-lookup-table.c:2
+// Check switch to lookup optimization in fPIC and fno-PIC mode
+// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O2 -fno-PIC -S -emit-llvm -o - | FileCheck %s --check-prefix=CHECK --check-prefix=FNOPIC
+// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O2 -fPIC -S -emit-llvm -o - | FileCheck %s --check-prefix=CHECK --check-prefix=FPIC
----------------
hans wrote:
> gulfem wrote:
> > hans wrote:
> > > Clang codegen tests are not normally used to test LLVM optimizations. I think the tests for this should all live in LLVM, not Clang.
> > > 
> > > (Also aarch64 is not guaranteed to be included as a target in the LLVM build, so this test would not necessarily run.)
> > I'm not able to use -fPIC and -fno-PIC options in the `opt` tool.
> > I am setting the `PIC Level` flag to enable -fPIC in `opt.
> > I thought that testing -fPIC and -fno-PIC in the same file seems easier and more readable  in CodeGen tests.
> > Please let me know if you have a good suggestion how to do that with `opt`.
> > 
> > I changed the target to `x86_64-linux` in this test.
> Buildbots may not have x86_64 as a registered target, so this test will break some buildbots.
> 
> I think the opt flags -relocation-model=pic and -relocation-model=static will do the right thing (see for example llvm/test/Transforms/SimplifyCFG/ARM/switch-to-lookup-table.ll
I added `x86-registered-target` to ensure that it only runs on buildbots that have `x86_64` as a registered target


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:39
+
+  // If not in PIC mode, do not generate a relative lookup table.
+  if (M.getPICLevel() == PICLevel::NotPIC)
----------------
hans wrote:
> Again, this needs the "why".
> And perhaps it would make sense to put this check first.
I added the reason, please let me know if that's not clear enough.


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:169
+
+  for (Module::global_iterator GVI = M.global_begin(), E = M.global_end();
+       GVI != E;) {
----------------
hans wrote:
> This would be simpler as
> 
> ```
> for (GlobalVariable *GlobalVar : M.globals()) {
> ```
Please keep that in mind that I'm deleting a global variable while I'm iterating over global variables.
I don't want to have invalidated iterator, and this is why I did not use a simple for loop.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94355/new/

https://reviews.llvm.org/D94355