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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked 3 inline comments as done.
lebedev.ri added inline comments.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1773
+  // Usually `extractvalue` is modelled as free.
+  if (TTI.getUserCost(&I, TargetTransformInfo::TCK_SizeAndLatency) ==
+      TargetTransformInfo::TCC_Free)
----------------
aeubanks wrote:
> lebedev.ri wrote:
> > xbolva00 wrote:
> > > Just question: this check is also in visitInstruction so maybe you can just return visitInstruction(I)?
> > Not really, they have different side-effects.
> do you mean the `disableSROA`? shouldn't that be happening anyway? or else we should handle SROA for extractvalue.
> do you mean the `disableSROA`?

Yes.

> shouldn't that be happening anyway? or else we should handle SROA for extractvalue.

Well, not as per the comment at the end of this function:
```
  // SROA can look through these but give them a cost.
  return false;
```
Regardless of whether or not that is wrong, it seems like a separate problem for another patch.

Regardless, deferring to `visitInstruction()` seems wrong,
because i think `getUserCost()` is much cheaper than the `simplifyInstruction()`
that would run first.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1787
 
 bool CallAnalyzer::visitInsertValue(InsertValueInst &I) {
   // Constant folding for insert value is trivial.
----------------
aeubanks wrote:
> should we do the same for insertvalue?
For consistency - maybe.
To model them as free - are `insertvalue`s free? D66098 only modelled `extractvalues`.
So again, seems like a separate issue.


================
Comment at: llvm/test/Transforms/Inline/X86/extractvalue.ll:1
+; RUN: opt < %s -inline -inline-threshold=0 -debug-only=inline-cost -print-instruction-comments -S -mtriple=x86_64-unknown-linux-gnu 2>&1 | FileCheck %s
+; RUN: opt < %s -passes='cgscc(inline)' -inline-threshold=0 -debug-only=inline-cost -print-instruction-comments -S -mtriple=x86_64-unknown-linux-gnu 2>&1 | FileCheck %s
----------------
aeubanks wrote:
> can the first `RUN` be deleted?
I'm only following the example in other tests in the directory.
I think if it's not needed, then perhaps all other such runlines should be dropped first?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101228