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

Thu Feb 18 05:14:41 PST 2021

hans added a comment.

Thanks for pushing this forward! I think this will be a nice transformation once all the details are worked out.

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

================
Comment at: llvm/include/llvm/InitializePasses.h:321
 void initializeNameAnonGlobalLegacyPassPass(PassRegistry&);
+void initializeRelLookupTableConverterPassPass(PassRegistry &);
 void initializeUniqueInternalLinkageNamesLegacyPassPass(PassRegistry &);
----------------
In some places the pass is referred to as a generator and here it's a converter. I think converter is a better name, and it should be consistent.

================
Comment at: llvm/include/llvm/Transforms/Utils/RelLookupTableGenerator.h:10
+// This file implements relative lookup table generator that converts
+// lookup tables to relative lookup tables to make them PIC-friendly.
+//
----------------
For this kind of pass, I think it would be helpful to have a small example in the comment that shows what kind of IR it's looking for, and what it will be transformed to. (Either here or in the .cpp file.)

================
Comment at: llvm/include/llvm/Transforms/Utils/RelLookupTableGenerator.h:22
+
+// Simple pass that converts lookup tables to relative lookup tables.
+class RelLookupTableGeneratorPass
----------------
No need to call it simple :)

================
Comment at: llvm/include/llvm/Transforms/Utils/RelLookupTableGenerator.h:33
+
+#endif // LLVM_TRANSFORMS_RELLOOKUPTABLEGENERATOR_H
----------------
As clang-tidy points out, the comment doesn't match the actual macro name.

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:25
+
+namespace llvm {
+
----------------
Since the function below are only used within this file, they should be static or in an anonymous namespace. Since you're already using an anonymous namespace for the RelLookupTableConverterPass class, I'd suggest using that for these functions too.

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:28
+bool shouldGenerateRelLookupTables(Module &M) {
+  // If not in x86 or aarch64 mode, do not generate a relative lookup table.
+  Triple TargetTriple(M.getTargetTriple());
----------------
This explains the "what" of what the code does, but not the "why". Why should the transformation only run for these two targets?

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:34
+
+  // If not tiny or small code model, do not generate a relative lookup table.
+  Optional<CodeModel::Model> CodeModel = M.getCodeModel();
----------------
This also needs the "why".

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:39
+
+  // If not in PIC mode, do not generate a relative lookup table.
+  if (M.getPICLevel() == PICLevel::NotPIC)
----------------
Again, this needs the "why".
And perhaps it would make sense to put this check first.

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:58
+    GlobalVariable *GlobalVarOp = dyn_cast<GlobalVariable>(Operand);
+    /// If any of the pointer values in the lookup table is not a global value
+    /// or dso_local, do not generate a relative lookup table.
----------------
The comment doesn't match the code exactly I think, since further down you also allow GetElementPtr expressions. Maybe the comment could be clearer.

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:78
+  /// do not generate a relative lookup table.
+  if (!GlobalVar.hasOneUser())
+    return false;
----------------
Should it be hasOneUse() or hasOneUser()?

Also maybe this check could some before the for-loop, since it's cheaper and may return early.

There probably also needs to be a check that the global has local linkage, otherwise it could be referenced outside the module.

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:88
+  ConstantArray *LookupTableArr =
+      dyn_cast<ConstantArray>(LookupTable.getInitializer());
+  unsigned NumElts = LookupTableArr->getType()->getNumElements();
----------------
if you know that the initializer is a ConstantArray, you can use cast<> instead of dyn_cast<> here

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:91
+  ArrayType *IntArrayTy =
+      ArrayType::get(Type::getInt32Ty(Mod.getContext()), NumElts);
+  GlobalVariable *RelLookupTable =
----------------
The int32 type here has been mentioned in reviews before.

I think at least, the code needs to have a good motivation for why 32 bits is enough.

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:119
+
+GlobalVariable *canGenerateRelLookupTable(GlobalVariable &LookupTable) {
+  User *U = LookupTable.use_begin()->getUser();
----------------
The "can" part of the name seems misleading, since this doesn't return true or false, but actually tries to build the lookup table (and possibly returns null if it can't). I'd just drop "can" from the name.

Oh, but there is already a generateRelLookupTable() function.. well, maybe there is a better name for one of them.

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:122
+  GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(U);
+  if (!GEP)
+    return nullptr;
----------------
This code which checks that the user of the table is a GEP, followed by a load, etc. feels like a continuation of the checks in shouldGenerateRelLookupTableForGlobal(). I would suggest just moving those checks into this function.

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:125
+
+  LoadInst *Load = dyn_cast<LoadInst>(GEP->getNextNode());
+  if (!Load)
----------------
getNextNode() doesn't seem right. It gets the next instruction, but that is not necessarily the same as the instruction using the GEP.

Also, the code probably needs to check that the GEP only has one use

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:129
+
+  Module &Mod = *LookupTable.getParent();
+  BasicBlock *BB = GEP->getParent();
----------------
(nit: commonly, llvm code uses an M variable for the module)

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:166
+
+  if (!shouldGenerateRelLookupTables(M))
+    return false;
----------------
I'd suggest moving this to the top of the function, before even declaring Changed.

================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableGenerator.cpp:169
+
+  for (Module::global_iterator GVI = M.global_begin(), E = M.global_end();
+       GVI != E;) {
----------------
This would be simpler as

```
for (GlobalVariable *GlobalVar : M.globals()) {
```

================
Comment at: llvm/test/Transforms/SimplifyCFG/X86/relative_lookup_table.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -rel-lookup-table-generator -S -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
+; RUN: opt < %s -passes=rel-lookup-table-generator -S -mtriple=x86_64-unknown-linux-gnu | FileCheck %s
----------------
(you could skip the -mtriple argument here since there is a "target triple" line in the IR below)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355