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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

overall looks good now, just nits



================
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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101228