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
+
+@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
