[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 and 4 inline comments as not done.
lebedev.ri added a comment.

@aeubanks thank you for taking a look!



================
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:
> > 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.
> If we first check `getUserCost()` then we may miss out on some simplification that `CallAnalyzer::simplifyInstruction()` could do for future users of the extractelement. It seems more in line (hehe) with the rest of CallAnalyzer to call `simplifyInstruction()` first. Lots of CallAnalyzer methods immediately call `simplifyInstruction()`.
> 
> Also, looking at `SROA.cpp`, it doesn't seem to handle extractelements.
Aha, okay, let's do that then.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1787
 
 bool CallAnalyzer::visitInsertValue(InsertValueInst &I) {
   // Constant folding for insert value is trivial.
----------------
aeubanks wrote:
> lebedev.ri wrote:
> > 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.
> oh, of course insertvalues aren't free, ignore my comment
Note that i'm not saying that they are/aren't free.
I'm only saying that currently they are modelled as non-free.
I *suspect* they may also be free, but they are so rare
that i haven't seen a motivational example including them...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101228