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