[PATCH] D94355: [SimplifyCFG] Add relative switch lookup tables

[Gulfem Savrun Yeniceri via Phabricator via llvm-commits]

gulfem marked 9 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:
> 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.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5709
+  return llvm::ConstantExpr::getTrunc(RelOffset,
+                                      Type::getInt32Ty(M.getContext()));
 }
----------------
hans wrote:
> I do worry about hard-coding this to 32 bits. As someone pointed out, it would not necessary hold in all code models for x86.
> 
> Similarly to the PIC check in ShouldBuildRelLookupTable(), is there some check we could do to make sure 32 bits is appropriate?
I added `x86_64` and `aarch64` target check and tiny or small code mode check to ensure 32 offsets.
Please let me know if you have any other concerns about that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355