[PATCH] D99302: [InlineCost] Make cost-benefit decision explicit

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:2618
   // Check if there was a reason to force inlining or no inlining.
+  if (ShouldInline.isSuccess() && CA.wasDecidedByCostBenefit())
+    return InlineCost::getAlways("benefit over cost");
----------------
wenlei wrote:
> davidxl wrote:
> > wenlei wrote:
> > > davidxl wrote:
> > > > Should this be folded into line 2622? 
> > > We wanted any positive cost-benefit decision to be distinguishable from a regular cost-threshold decision. This is because sample loader inlining looks at cost, and compare against its own threshold (`SampleProfileLoader::shouldInlineCandidate`). But we want cost-benefit decision to take precedence there too. I can give the three flat if some structure.. 
> > > 
> > > Perhaps it'd be good to make negative cost-benefit decision distinguishable too, by using never. An alternative is to add a field in InlineCost indicating a decision is made through cost-benefit analysis.
> > what I meant is re-organize your change a little:
> > 
> > if (ShouldInline.isSuccess()) {
> >    if (CA.wasDecidedByCostBenefit() {
> >     ...
> >    } 
> >    if (CA.getCost() >= CA.getThreshold() ) {
> >    }
> > 
> > }
> How about this above the existing checks. On 2nd thought, I think it makes sense to make negative cost-benefit decision explicit too.
> 
> ```
>   // Always make cost benefit based decision explicit.
>   // We use always/never here since threshold is not meaningful,
>   // as it's not what drives cost-benefit analysis.
>   if (CA.wasDecidedByCostBenefit()) {
>     if (ShouldInline.isSuccess())
>       return InlineCost::getAlways("benefit over cost");
>     else
>       return InlineCost::getNever("cost over benefit");
>   }
> ```
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99302