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

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

> Based on @hans 's feedback, I added a new enumeration kind called `RelOffsetArrayKind`, and remove `IsRelative` flag.
> This separates the `lookup table` and `relative lookup table` IR generation.
> Do you think this helps for the abstraction?
> I feel like if it might be better if we can generate the relative lookup table in the first place instead of generating it in a separate pass.

The benefit of doing it as a separate optimization would be that it could apply to more than just switch statements. For example, perhaps it could also transform this code:

  const char *f(int x) {
    static const char *const tbl[] = { "foo", "bar", "baz" };
    return tbl[x];
  }

I don't know how hard this would be, but maybe it's worth investigating?



================
Comment at: llvm/include/llvm/Transforms/Utils/SimplifyCFGOptions.h:27
   bool ConvertSwitchToLookupTable = false;
+  bool RelativeSwitchLookupTable = false;
   bool NeedCanonicalLoop = true;
----------------
gulfem wrote:
> hans wrote:
> > Is it necessary to have an option for this? Can't SwitchToLookupTable just decide itself whether to do this, based on whether it's generating PIC code or not?
> > 
> > (In any case, I think this only makes sense for PIC code, right?)
> Yes, this is only intended for PIC code.
> The option is not necessary, but I added it for testing purposes.
I would suggest dropping the option unless it's necessary. I think the transformation can still be tested without it.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5502
+
+    // The table is stored as an array of relative offsets.
+    // Values are retrieved by load and add instructions from the table.
----------------
nit: offsets relative to what?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5522
+  // For RelOffsetArrayKind, this is the array.
+  GlobalVariable *RelOffsetArray = nullptr;
 };
----------------
Maybe it would be easier to use Array for both ArrayKind and RelOffsetArrayKind instead of having a separate variable?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:5543
+  /// elements of the lookup table.
+  if (EnableRelativeTable && ValueType->isPointerTy()) {
+    ArrayType *ArrayTy = ArrayType::get(Builder.getInt32Ty(), TableSize);
----------------
+1 I think we'd need to check that the pointers are dso_local (or convince ourselves that it can't happen)


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355