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

[Wenlei He via Phabricator via llvm-commits]

wenlei 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");
----------------
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");
  }
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99302