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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5534
+  /// elements of the of the lookup table
+  if (EnableRelativeTable && ValueType->isPointerTy()) {
+    IsRelative = true;
----------------
Since this constructor can technically take a vector of any arbitrary constants, perhaps we should also add a check before this making sure that if the values are pointers, then we should expect all pointee values to be `dso_local` for this to work. That is, if the switch/phi nodes were to accept pointers to strings that were defined externally, then we wouldn't emit a relative table since the arithmetic wouldn't work.

It could be that this case is unlikely and I'm just nitpicking, but if this were a case that could happen in the future then it might be nice to catch it now.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5552-5559
+      Constant *Base =
+          llvm::ConstantExpr::getPtrToInt(Array, Builder.getInt64Ty());
+      Constant *Target =
+          llvm::ConstantExpr::getPtrToInt(CaseRes, Builder.getInt64Ty());
+      llvm::Constant *RelOffset = llvm::ConstantExpr::getSub(Target, Base);
+      ;
+      RelOffset = llvm::ConstantExpr::getTrunc(RelOffset, Builder.getInt32Ty());
----------------
Could you put this in a lambda or static function? I believe this could be reused for lines 5575-5584.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5557
+      llvm::Constant *RelOffset = llvm::ConstantExpr::getSub(Target, Base);
+      ;
+      RelOffset = llvm::ConstantExpr::getTrunc(RelOffset, Builder.getInt32Ty());
----------------
Extra `;`


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5576-5583
+          Constant *Base =
+              llvm::ConstantExpr::getPtrToInt(Array, Builder.getInt64Ty());
+          Constant *Target = llvm::ConstantExpr::getPtrToInt(
+              DefaultValue, Builder.getInt64Ty());
+          llvm::Constant *RelOffset = llvm::ConstantExpr::getSub(Target, Base);
+          RelOffset =
+              llvm::ConstantExpr::getTrunc(RelOffset, Builder.getInt32Ty());
----------------
Since the `DefaultValue` is changed in this loop, I think this could probably be moved outside and before the loop.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5657
+  // If relative table is generated, initialize the array with the table
+  if (EnableRelativeTable && ValueType->isPointerTy()) {
+    ArrayType *ArrayTy = ArrayType::get(Builder.getInt32Ty(), TableSize);
----------------
Nit: `if (IsRelative) {`


================
Comment at: llvm/test/Transforms/SimplifyCFG/X86/switch_to_relative_lookup_table.ll:18
+; Relative string table lookup
+; CHECK: @switch.table.string_relative_table = private unnamed_addr constant [9 x i32] [i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str to i64), i64 ptrtoint ([9 x i32]* @switch.table.string_relative_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.1 to i64), i64 ptrtoint ([9 x i32]* @switch.table.string_relative_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.2 to i64), i64 ptrtoint ([9 x i32]* @switch.table.string_relative_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([6 x i8]* @.str.3 to i64), i64 ptrtoint ([9 x i32]* @switch.table.string_relative_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str.4 to i64), i64 ptrtoint ([9 x i32]* @switch.table.string_relative_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([5 x i8]* @.str.5 to i64), i64 ptrtoint ([9 x i32]* @switch.table.string_relative_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([4 x i8]* @.str.6 to i64), i64 ptrtoint ([9 x i32]* @switch.table.string_relative_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([6 x i8]* @.str.7 to i64), i64 ptrtoint ([9 x i32]* @switch.table.string_relative_table to i64)) to i32), i32 trunc (i64 sub (i64 ptrtoint ([6 x i8]* @.str.8 to i64), i64 ptrtoint ([9 x i32]* @switch.table.string_relative_table to i64)) to i32)], align 8
+
----------------
Should the alignment be 4 on this array and the array below?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355