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