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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri accepted this revision.
lebedev.ri added a comment.

Thank you!



================
Comment at: llvm/include/llvm/CodeGen/BasicTTIImpl.h:390-391
+
+    if (!TM.getTargetTriple().isArch64Bit())
+      return false;
+
----------------
gulfem wrote:
> lebedev.ri wrote:
> > gulfem wrote:
> > > lebedev.ri wrote:
> > > > 1. But all tests are using `x86_64` triple?
> > > > 2. This is somewhat backwards. if the target wants to disable this, it will need to override this function with `return false;`.
> > > 1. Although I used `x86_64 triple`, this optimization can be applied to other 64-bit architectures too, because it not target dependent except `isArch64Bit` and `getCodeModel` check.
> > > 2. Is there a target that you have in mind that we need to disable this optimization? 
> > > I thought that it makes sense to enable this optimization by default on all the targets that can support it.
> > > In case targets want to disable it, they can override it as you said.
> > > How can we improve the implementation?
> > > If you have suggestions, I'm happy to incorporate that.
> > > 
> > I'm sorry, i do not understand.
> > Why does `!TM.getTargetTriple().isArch64Bit()` check exist?
> > To me it reads as "if we aren't compiling for AArch64, don't build rel lookup tables".
> > Am i misreading this?
> `isArch64Bit` checks whether we have a 64-bit architecture, right?
> I don't think it specifically checks for `AArch64`, and it can cover other 64-bit architectures like `x86_64` as well.
> isArch64Bit checks whether we have a 64-bit architecture, right?

D'oh. I really did read it as A*A*rch64 :/ Sorry.


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:172-173
+
+  for (auto GVI = M.global_begin(), E = M.global_end(); GVI != E;) {
+    GlobalVariable &GlobalVar = *GVI++;
+
----------------
`make_early_inc_range()`?


================
Comment at: llvm/lib/Transforms/Utils/RelLookupTableConverter.cpp:200
+
+  return PreservedAnalyses::none();
+}
----------------
I would think this should be
```
PreservedAnalyses PA;
PA.preserveSet<CFGAnalyses>();
return PA;
```
since this doesn't touch CFG at all.
I think this should get rid of redundant `Running analysis: TargetIRAnalysis`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94355