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

[Hans Wennborg via Phabricator via llvm-commits]

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
+
+ at .str = private unnamed_addr constant [5 x i8] c"zero\00", align 1
+ at .str.1 = 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