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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked an inline comment as done.
lebedev.ri added a comment.

@aeubanks thank you for taking a look!
The nit is non-addressable, so no changes are made.



================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1782
+  // SROA can't look through these, but they may be free.
+  return visitInstruction(I);
 }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101228