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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

It looks like you have everything setup for running on the new PM, but it doesn't look like the pass is added anywhere in the new PM pipeline. Unfortunately, I don't *think* there's anything in the new PM that acts as a "default pipeline that gets added to all other pipelines" similar to `PassManagerBuilder::populateModulePassManager`, so we'll need to manually include this somewhere in `PassBuilder::buildO0DefaultPipeline` and `PassBuilder::buildPerModuleDefaultPipeline`.

Depending on if we also want to support this in [thin]LTO, we may need to add this to more pipelines.



================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:46
+
+bool shouldGenerateRelLookupTableForGlobal(GlobalVariable &GlobalVar) {
+  if (!GlobalVar.hasInitializer() ||
----------------
We should also check if the switch table itself is dso_local since the right relocation won't be generated if it isn't.


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:51
+
+  ConstantArray *Array = dyn_cast<ConstantArray>(GlobalVar.getInitializer());
+  // If values are not pointers, do not generate a relative lookup table.
----------------
`cast` since we checked before this is a ConstantArray. Alternatively, what you could have is:

```
if (!GlobalVar.hasInitializer())
  return false;

ConstantArray *Array = dyn_cast<ConstantArray>(GlobalVar.getInitializer());
if (!Array || !Array->getType()->getElementType()->isPointerTy())
  return false;
```


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:61
+    if (GlobalVarOp &&
+        !(GlobalVarOp->isDSOLocal() || GlobalVarOp->hasLocalLinkage()))
+      return false;
----------------
Rather than checking the linkage explicitly, you can use `isImplicitDSOLocal` which also has some visibility checks.

```
GlobalVarOp->isDSOLocal() || GlobalVarOp->isImplicitDSOLocal()
```


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:64-70
+    ConstantExpr *CE = dyn_cast<ConstantExpr>(Operand);
+    if (!CE || CE->getOpcode() != Instruction::GetElementPtr)
+      return false;
+
+    GlobalValue *Pointer = dyn_cast<GlobalValue>(CE->getOperand(0));
+    if (!Pointer)
+      return false;
----------------
It seems that with this, we're limiting this to only arrays with GEPs with globals as the base, but I think this will return false if the array element is just a dso_local global. We definitely should still be taking into account GEPs though.

I'm thinking `IsConstantOffsetFromGlobal` might be more useful here since it already contains a bunch of logic for handling ConstantExpr GEPs, then you can check if the global found by that is dso_local.


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:76-77
+
+  /// If lookup table has more than one user,
+  /// do not generate a relative lookup table.
+  if (!GlobalVar.hasOneUser())
----------------
What's the reason for why the number of users matters?


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:92-95
+  GlobalVariable *RelLookupTable =
+      new GlobalVariable(Mod, IntArrayTy,
+                         /*isConstant=*/true, GlobalVariable::PrivateLinkage,
+                         nullptr, "reltable." + Func.getName());
----------------
I think the visibility and linkage should be set the same as those of the original lookup table.

I think to avoid many changes to the original lookup table and only focus on the new layout, we should also propagate any properties of the original table to the new relative table. That is, visibility, linkage, attributes, unnamed_addr, etc. should be copied from the original.

(For copying attributes, you can use `copyAttributesFrom`.)


================
Comment at: llvm/test/Transforms/SimplifyCFG/X86/relative_lookup_table.ll:31
+
+ at switch.table.no_dso_local = private unnamed_addr constant [3 x i32*] [i32* @a, i32* @b, i32* @c], align 8
+
----------------
We should also have cases that cover other linkages/visibilities:
- If the table elements are `extern dso_local` or `extern hidden`, we should still expect a relative lookup table
- If the switch table is not dso_local/hidden, we shouldn't expect a relative lookup table


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355