[PATCH] D63058: [InlineCost] Fix bug 42084: return the first negative result

[Fedor Sergeev via Phabricator via llvm-commits]

fedor.sergeev added a comment.

In D63058#1537270 <https://reviews.llvm.org/D63058#1537270>, @chandlerc wrote:

> I think this is somewhat the wrong approach.
>  I think we need to widen the interface a bit to return two pieces of information instead of one:




In D63058#1553186 <https://reviews.llvm.org/D63058#1553186>, @yrouban wrote:

> I still believe that we should separate a bug fix from introducing a new feature. As I read the comments this patch looks to be complex to be just a bugfix but not full enough to introduce a new and complete feature.
>  I could provide a one line fix with a test if I believe it is a complete fix for the bug. In other words I'm a bit stuck. Any suggestions are welcome.


I kinda agree both with you and Chandler.
As Chandler mentioned (and I'm just rephrasing in a slightly different manner), there appear to be two "informational" axis for the *effect* of CallAnalyzer (whether being returned by InlineResult or by side-effects of remarks etc):

1. binary result of cost-vs-threshold calculation
2. "full" result of the analysis performed by CallAnalyzer on function body

As a matter of fixing PR42084 you are rightfully worried about 1 (being wrong in some cases).
As a matter of overall consistency, clarity of representation and algorithm control, Chandler is worried about "full" result not presented directly, so it is harder to reason about it.

It appears that in my current downstream experiments with inliner and InlineCost I need more sophisticated control both on 1. and 2. (doing more than just Cost < Threshold and also collecting more information as part of the analysis - e.g. tracking separately devirtualization effects, etc).
For that to be done with less conflicts with upstream I was recently contemplating the idea of making InlineResult more sophisticated.

That kinda aligns with the direction that Chandler outlined, so here is my suggestion:

- if you have a one-liner fix - lets take a look, I'm not sure what do you mean here exactly
- otherwise, I'm gonna try to kraft an alternative fix that extends InlineResult with ability to:
  - be notified about different "inline result actions"  (e.g. terminal conditions)
  - be responsible for making a decision on whether to continue analysis or stop (handling 1.)
  - be responsible for bookkeeping of actions (handling 2.)

Deal?


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

https://reviews.llvm.org/D63058