hans 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
----------------
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.)
================
Comment at: clang/test/CodeGen/switch-to-lookup-table.c:6
+// Switch lookup table
+// FNOPIC: @switch.table.string_table = private unnamed_addr constant [9 x
i8*] [i8* getelementptr inbounds ([5 x i8], [5 x i8]* @.str, i64 0, i64 0), i8*
getelementptr inbounds ([4 x i8], [4 x i8]* @.str.1, i64 0, i64 0), i8*
getelementptr inbounds ([4 x i8], [4 x i8]* @.str.2, i64 0, i64 0), i8*
getelementptr inbounds ([6 x i8], [6 x i8]* @.str.3, i64 0, i64 0), i8*
getelementptr inbounds ([5 x i8], [5 x i8]* @.str.4, i64 0, i64 0), i8*
getelementptr inbounds ([5 x i8], [5 x i8]* @.str.5, i64 0, i64 0), i8*
getelementptr inbounds ([4 x i8], [4 x i8]* @.str.6, i64 0, i64 0), i8*
getelementptr inbounds ([6 x i8], [6 x i8]* @.str.7, i64 0, i64 0), i8*
getelementptr inbounds ([6 x i8], [6 x i8]* @.str.8, i64 0, i64 0)], align 8
+
----------------
This table and the one below are very hard to read like this. Could you split
it into multiple lines using FNOPIC-SAME?
================
Comment at: clang/test/CodeGen/switch-to-lookup-table.c:36
+
+ switch (cond) {
+ case 0:
----------------
I think the minimum number of cases for the switch-to-lookup table
transformation is only 4 or 5. To make the test easier to read, I'd suggest
using the minimum number of cases in the switch.
================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5658
+
+ // If relative table is generated, initialize the array with the table.
+ Constant *Initializer = ConstantArray::get(ArrayTy, TableContents);
----------------
This comment seems unnecessary, at this point we know we're generating the
relative table.
================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5695
+ if (auto *GlobalVal = dyn_cast<GlobalValue>(CaseRes))
+ if (!GlobalVal->isDSOLocal())
+ return false;
----------------
I don't remember, will isDSOLocal() return true also if it's a private or
internal symbol? Otherwise maybe this should check isLocalLinkage() also.
================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5709
+ return llvm::ConstantExpr::getTrunc(RelOffset,
+ Type::getInt32Ty(M.getContext()));
}
----------------
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?
================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5712
-Value *SwitchLookupTable::BuildLookup(Value *Index, IRBuilder<> &Builder) {
+Value *SwitchLookupTable::BuildLookup(Module &M, Value *Index, Type *ValueType,
+ IRBuilder<> &Builder) {
----------------
The Builder points to a specific insertion point in a basic block for the
lookup, so it knows the Module and adding the Module parameter is redundant.
================
Comment at:
llvm/test/Transforms/SimplifyCFG/X86/switch_to_relative_lookup_table.ll:7
+
[email protected] = private unnamed_addr constant [5 x i8] c"zero\00", align 1
[email protected] = private unnamed_addr constant [4 x i8] c"one\00", align 1
----------------
Same comment as I made for the test under clang/ above: I think fewer switch
cases are probably enough to test this, and would make it easier to read. Also
splitting the lookup tables over multiple lines would help too.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D94355/new/
https://reviews.llvm.org/D94355
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
