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

[Hans Wennborg via Phabricator via llvm-commits]

hans added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5469
+  /// Otherwise, returns false.
+  static bool ShouldBuildRelLookupTable(
+      const SmallVectorImpl<std::pair<ConstantInt *, Constant *>> &Values);
----------------
Since this is a helper function used internally by the class, it should be private.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5473
+  /// Generate an offset between the lookup table and given case result.
+  static Constant *GenerateRelOffset(GlobalVariable *Array, Constant *CaseRes,
+                                     IRBuilder<> &Builder);
----------------
Actually, this should probably be private too.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5701
+                                               IRBuilder<> &Builder) {
+  Constant *Base = llvm::ConstantExpr::getPtrToInt(Array, Builder.getInt64Ty());
+  Constant *Target =
----------------
It's a little annoying that we have to pass in a Builder here (and also to the SwitchLookupTable class) just to create integer types. I don't think a builder is strictly necessary for that.

But more importantly, I'm not sure hard-coding this to 64 bits for the pointer and 32 bits for the offset is correct. Shouldn't this depend on the target?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5740-5747
+    // Make sure the table index will not overflow when treated as signed.
+    IntegerType *IT = cast<IntegerType>(Index->getType());
+    uint64_t TableSize =
+        RelOffsetArray->getInitializer()->getType()->getArrayNumElements();
+    if (TableSize > (1ULL << (IT->getBitWidth() - 1)))
+      Index = Builder.CreateZExt(
+          Index, IntegerType::get(IT->getContext(), IT->getBitWidth() + 1),
----------------
leonardchan wrote:
> Nit: Since this seems to be the exact same as the start of the `ArrayKind` case, it might be cleaner to move this into a lambda or function.
One alternative would also be to bake the cases together:

```
case ArrayKind:
case RelOffsetArrayKind: {
... common stuff ...
if (Kind == RelOffsetArrayKind) {
  .. special stuff ..
}
```



================
Comment at: llvm/test/Transforms/SimplifyCFG/ARM/switch-to-relative-lookup-table.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -simplifycfg -simplifycfg-require-and-preserve-domtree=1 -switch-to-lookup -relative-switch-lookup-table -mtriple=arm -relocation-model=pic       < %s | FileCheck %s
----------------
gulfem wrote:
> hans wrote:
> > Any reason to have tests both under ARM/ and X86/ for this?
> > 
> > Also I'd like to see a test which shows that the transformation happens for PIC code but not for non-PIC.
> > Any reason to have tests both under ARM/ and X86/ for this?
> There are two existing switch-to-lookup tests one under ARM/ and one under /X86 (I don't know the original reason).
> /ARM test is much simpler. I now put all the relative lookup tests under /X86.
> 
> > Also I'd like to see a test which shows that the transformation happens for PIC code but not for non-PIC.
> Could you please clarify that? 
> Do you want to see a test that checks the code for non-PIC case when -fno-PIC is enabled?
> 
> 
> Do you want to see a test that checks the code for non-PIC case when -fno-PIC is enabled?

Yes, exactly. This seems important since we don't want the transformation to run for the no-PIC case (and currently I don't see anything in the code which ensures that.)

It might help to put the relative lookup table test in a separate file, and use two invocations once with PIC and one without, and check the expected results with different FileCheck prefixes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355