[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:
> > 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() ) {
   }

}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99302