[PATCH] D101228: [InlineCost] CallAnalyzer: use TTI info for extractvalue - they are free (PR50099)

[Arthur Eubanks via Phabricator via llvm-commits]

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