[PATCH] D101228: [InlineCost] CallAnalyzer: use TTI info for extractvalue - they are free (PR50099)
Arthur Eubanks via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 29 14:58:02 PDT 2021
aeubanks added inline comments.
================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1782
+ // SROA can't look through these, but they may be free.
+ return visitInstruction(I);
}
----------------
lebedev.ri wrote:
> aeubanks wrote:
> > lebedev.ri wrote:
> > > aeubanks wrote:
> > > > I think `Base::visitExtractValue()` is the proper way
> > > Well, then all the code that uses this "proper way" is likely broken,
> > > and either doesn't have test coverage,
> > > or the test changes weren't analyzed well enough.
> > >
> > > Because then we don't fall back to `CallAnalyzer::visitInstruction()`,
> > > and don't consider it as zero-cost as per costmodel...
> > I thought `Base::visitExtractValue()` ends up calling `CallAnalyzer::visitInstruction()`. `Base::visitExtractValue()` goes through InstVisitor's instruction hierarchy to figure out what to call next. Currently it always ends up calling `CallAnalyzer::visitInstruction()`, but if in the future InstVisitor put something in between `visitExtractValue()` and `visitInstruction()` then we'd want to respect that.
> > I thought `Base::visitExtractValue()` ends up calling `CallAnalyzer::visitInstruction()`.
>
> That is not what is happening, no.
> Moreover, i'm not sure `InstVisitor` would ever do that.
I took a closer look, InstVisitor does do something like that, but in this case it calls `CallAnalyzer::visitUnaryInstruction()` before getting to `visitInstruction()`. If we delete `CallAnalyzer::visitUnaryInstruction()` what I suggested should work. D101577 to remove `visitUnaryInstruction()`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D101228/new/
https://reviews.llvm.org/D101228
More information about the llvm-commits
mailing list