Page MenuHomePhabricator

[InlineCost] Fix bug 42084: return the first negative result
Needs ReviewPublic

Authored by yrouban on Jun 9 2019, 7:21 AM.

Details

Summary

Full inline cost calculation results in visiting more instructions. This may result in getting another cause of a bailout or mistakenly forget that there was a negative result where Cost > Threshold.
This patch makes InlineCost consistent in returning the same InlineResult regardless of the flag ComputeFullInlineCost.
This patch (particularly the part in CallAnalyzer::analyzeBlock()) fixes the bug https://bugs.llvm.org/show_bug.cgi?id=42084

Diff Detail

Event Timeline

yrouban created this revision.Jun 9 2019, 7:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2019, 7:21 AM

Thanks for looking at this bug.

I commited test case https://reviews.llvm.org/rL362906. Can you rebase now? Thanks.

I added some more reviewers..

2 questions, since i've read through the diff:

  1. will this pick the smallest cost, or the first negative cost?
  2. is this missing some abstraction? maybe InlineResult Result should be some wrapper that should be assigned-to, that will internally only accept the new value if it's better than what it currently has,

tests?

Makes sense. I will try.

2 questions, since i've read through the diff:

  1. will this pick the smallest cost, or the first negative cost?
  2. is this missing some abstraction? maybe InlineResult Result should be some wrapper that should be assigned-to, that will internally only accept the new value if it's better than what it currently has,
  1. Not the smallest cost, but the first negative result with its message.
  2. I think such an abstraction would not be much shorter: Result = InlineResult::FirstNegative(Result, expression)

2 questions, since i've read through the diff:

  1. will this pick the smallest cost, or the first negative cost?
  2. is this missing some abstraction? maybe InlineResult Result should be some wrapper that should be assigned-to, that will internally only accept the new value if it's better than what it currently has,
  1. Not the smallest cost, but the first negative result with its message.
  2. I think such an abstraction would not be much shorter: Result = InlineResult::FirstNegative(Result, expression)

I am in favour of some wrapper too + add a comment, explain first neg. result case, etc..
Current code is quite hard to follow what is going on..

chandlerc requested changes to this revision.Jun 10 2019, 6:07 PM

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:

  1. The highest cost computed
  2. Any inline-blocking construct encountered

And we should phrase #2 as a bitmask so that we can return multiple things in it. One of them can be that we exceeded the threshold, another can be any terminal condition we additionally reached like recursion.

I think this will also make the code more clear as we won't be throwing away any information at any step.

-Chandler

This revision now requires changes to proceed.Jun 10 2019, 6:07 PM
yrouban planned changes to this revision.Jun 11 2019, 3:19 AM

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:

  1. The highest cost computed
  2. Any inline-blocking construct encountered And we should phrase #2 as a bitmask so that we can return multiple things in it. One of them can be that we exceeded the threshold, another can be any terminal condition we additionally reached like recursion. I think this will also make the code more clear as we won't be throwing away any information at any step.

In general I agree with this approach. But the bigger change it needs the stronger intention I have to fix the bug with a minimal change and to develop the feature as another separate patch. One issue bothers me: results of the InlineCost analyzer will depend on the flag ComputeFullInlineCost. Negative or positive decision will persist but the message and highest computed cost will be changing.

yrouban updated this revision to Diff 204741.Jun 14 2019, 4:45 AM
  • extracted a separate function GetFirstNegative()
  • changed the test to check that the bug is fixed

One issue bothers me: results of the InlineCost analyzer will depend on the flag ComputeFullInlineCost. Negative or positive decision will persist but the message and highest computed cost will be changing.

We already have the cost difference depending on this flag.
It seems to be rather obvious consequence of "full inline cost" notion and the difference should not be confusing...

One issue bothers me: results of the InlineCost analyzer will depend on the flag ComputeFullInlineCost. Negative or positive decision will persist but the message and highest computed cost will be changing.

We already have the cost difference depending on this flag.
It seems to be rather obvious consequence of "full inline cost" notion and the difference should not be confusing...

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 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:

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?

xbolva00 resigned from this revision.Jul 19 2019, 12:39 PM