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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.

One thing that just occurred to me: do we also perhaps want to hide this behind a flag? Right now it's being added to various default optimization pipelines, so some users might be surprised if they suddenly see their lookup tables change (either compiler or user generated). Do we want to make this something more opt-in, or is the layout itself not necessarily a concern to most users?



================
Comment at: clang/test/CodeGen/switch-to-lookup-table.c:2
+// Check switch to lookup optimization in fPIC and fno-PIC mode
+// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O2 -fno-PIC -S -emit-llvm -o - | FileCheck %s --check-prefix=CHECK --check-prefix=FNOPIC
+// RUN: %clang_cc1 %s -triple=aarch64-unknown-fuchsia -O2 -fPIC -S -emit-llvm -o - | FileCheck %s --check-prefix=CHECK --check-prefix=FPIC
----------------
gulfem wrote:
> hans wrote:
> > gulfem wrote:
> > > hans wrote:
> > > > Clang codegen tests are not normally used to test LLVM optimizations. I think the tests for this should all live in LLVM, not Clang.
> > > > 
> > > > (Also aarch64 is not guaranteed to be included as a target in the LLVM build, so this test would not necessarily run.)
> > > I'm not able to use -fPIC and -fno-PIC options in the `opt` tool.
> > > I am setting the `PIC Level` flag to enable -fPIC in `opt.
> > > I thought that testing -fPIC and -fno-PIC in the same file seems easier and more readable  in CodeGen tests.
> > > Please let me know if you have a good suggestion how to do that with `opt`.
> > > 
> > > I changed the target to `x86_64-linux` in this test.
> > Buildbots may not have x86_64 as a registered target, so this test will break some buildbots.
> > 
> > I think the opt flags -relocation-model=pic and -relocation-model=static will do the right thing (see for example llvm/test/Transforms/SimplifyCFG/ARM/switch-to-lookup-table.ll
> I added `x86-registered-target` to ensure that it only runs on buildbots that have `x86_64` as a registered target
+1 on this. Unless this functionally changes something in the clang codebase, this test shouldn't be here. As hans pointed out, setting the `-relocation-model` should be enough.


================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:384
 
+  bool shouldBuildRelLookupTables() {
+    const TargetMachine &TM = getTLI()->getTargetMachine();
----------------
Sorry, I think you might have explained this offline, but what was the reason for needing this in TTI? I would've though this information could be found in the `Module` (PIC/no PIC, 64-bit or not, code model). If it turns out all of this is available in `Module`, then we could greatly simplify some logic here by just checking this at the start of the pass run.

If TTI is needed, then perhaps it may be better to just inline all these checks in `convertToRelativeLookupTables` since this is the only place this is called. I think we would only want to keep this here as a virtual method if we plan to have multiple TTI-impls overriding this.


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:880
 
+  MPM.add(createRelLookupTableConverterPass());
+
----------------
Should this be added at the end of the pipeline? It could be possible that other passes insert lookup tables but they may be untouched by this if this pass runs before them.


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:30-31
+
+  /// If lookup table has more than one user,
+  /// do not generate a relative lookup table.
+  if (!GlobalVar.hasOneUse())
----------------
Nit: `//`


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:40
+
+  if (!dyn_cast<LoadInst>(GEP->use_begin()->getUser()))
+    return false;
----------------
`isa`


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:53-70
+  for (Use &Operand : Array->operands()) {
+    if (GlobalVariable *GlobalVarOp = dyn_cast<GlobalVariable>(Operand)) {
+      /// If any of the pointer values in the lookup table is a global value
+      /// but not dso_local, do not generate a relative lookup table.
+      if (!(GlobalVarOp->isDSOLocal() || GlobalVarOp->isImplicitDSOLocal()))
+        return false;
+    } else {
----------------
I think I mentioned this in a previous round of comments, but what you probably want here is just a way to determine if the operand is either a global or some constant offset from global. Right now it looks this this will ignore other constant expressions like bitcasts or ptrtoints which we also want to catch. I think it might be better to use `IsConstantOffsetFromGlobal` which already handles these cases.


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:79
+  ConstantArray *LookupTableArr =
+      dyn_cast<ConstantArray>(LookupTable.getInitializer());
+  unsigned NumElts = LookupTableArr->getType()->getNumElements();
----------------
`cast`


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:218
+    };
+    convertToRelativeLookupTables(M, GetTTI);
+    return false;
----------------
Should we be returning the value returned by this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355