[PATCH] D94355: [Passes] Add relative lookup table converter pass

[Gulfem Savrun Yeniceri via Phabricator via llvm-commits]

gulfem added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:73
+      return false;
+
+    // If an operand in the lookup table is not dso_local,
----------------
pcc wrote:
> In the version of the patch that you committed, you have this check here:
> ```
>     // If operand is mutable, do not generate a relative lookup table.
>     auto *GlovalVarOp = dyn_cast<GlobalVariable>(GVOp);
>     if (!GlovalVarOp || !GlovalVarOp->isConstant())
>       return false;
> ```
> 1. Nit: Gloval -> Global
> 2. Why is it important whether the referenced global is mutable? The pointer itself is constant.
1. That's a typo, and I will fix that.
2. This optimization does not do a detailed analysis, and it is being conservative.
     In this case, `GlobalVar` points to the switch lookup table and `GlobalVarOp` points to the elements in the lookup table like strings.
     To make sure that relative arithmetic works, it just checks whether both `GlobalVar` and `GlobalVarOp` pointers are constants.
     Did you see an issue on that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355