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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
