[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:1773
+  // Usually `extractvalue` is modelled as free.
+  if (TTI.getUserCost(&I, TargetTransformInfo::TCK_SizeAndLatency) ==
+      TargetTransformInfo::TCC_Free)
----------------
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.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1787
 
 bool CallAnalyzer::visitInsertValue(InsertValueInst &I) {
   // Constant folding for insert value is trivial.
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101228