- Return from analyzeCall when inlining is known to be unviable (instead of breaking from the cost analysis loop).
- Move code that sets Threshold to zero to updateThreshold
- Prevent threshold from going negative.
I think the right fix is to move code in line 1217 -> 1237 into UpdateThreshold method.
Also it does not look good to update the threshold (subtracting singleBBbonus) within the loop. It should be safe to do that after the loop. To make the code more readable, it might be good to introduce two helper functions to addBonusToThreshold and subtractBonus ..
I'm confused with the meaning of threshold here.
In the loop which visits the subset of basic blocks in the function, we use this exit condition:
if (Cost > Threshold)
This indicates as long as Cost doesn't exceed Threshold, the function is still eligible for inlining. If the function's Cost is equal to Threshold, it can still be inlined.
However, the last statement indicates that Cost has to be smaller than Threshold in order for it to be inlined:
return Cost < std::max(1, Threshold);
Shouldn't the loop exit when Cost >= Threshold? In that case, the minimum value of Threshold should be 1, not 0, to enable inlining zero-cost functions.
At worst, this delays early bailout. If we change it to Cost >= Threshold, then we need to handle the special case of Cost == 0 and Threshold == 0. I don't like this special casing of 0 cost (the Cost <= std::max(1, Threshold) check), but reverting that would require investigating the size increase seen by Hans.
I wanted a generic name to decide whether to set Threshold to 0.
It's not strictly needed, but I think it makes the intention clear that Threshold is bounded below by 0.
Cost cannot be greater than Threshold in the first iteration - since there is a similar check in line 1245 above. Subsequently, if Cost exceeds Threshold, that's caught in analyzeBB which returns false and the bailout happens in that case.
I don't know why they were in the first place. If analyzeBlock returns false and it reaches the break stmt, it only means Cost > Threshold and we should be bailing out. There is no reason to break out of loop and return false at the end.
You have three only vaguely related changes here. In general, this should have been split up. I'm going to request that the third piece be split and reviewed separately. Once that's done, the first two LGTM w/noted changes as well.
Please make this an assertion since it's a non-obvious loop invariant.
This should be separated into it's own change. It isn't obvious to me that it's correct. (Not saying it isn't, just not obvious it is.) Separate this into it's own patch and add an assertion of the invariant you're asserting (i.e Threshold >= 0)
(Actually, you do that below. Still separate and separately review please.)
Is this comment only for this subtraction of SingleBBBonus or do you want me to move all changes of the form Threshold -=X => Threshold = std::max(Threshold - X, 0) into a separate patch? I think you mean the latter, but please clarify
Address Philip's comment and fix a bug discovered in the process.
There is a subtle bug in my reasoning that is caught by adding the assert as suggested by Philip. The invariant is maintained at the beginning of the first iteration. In the first iteration, if analyzeBlock returned true (Cost <= Threshold), Cost could still end up > Threshold when SingleBBBonus is subtracted, violating the invariant in the second iteration. What I have done now is to add a if (Cost > Threshold) return false immediately after the SingleBBBonus is subtracted. I think this is a bit more readable since the check is present immediately after the bonus is subtracted.
Sorry for the long delay.
Easwaran, please split the patches into smaller ones.
Checkd in this part. This actually fixes the issue Akira had because you won't end up with a positive bonus, but an initial zero threshold. But the complex control flow means there could be other unintended results.
- split out another NFC clean-up code (i.e. removal of IsRecursiveCall etc at line 1328)
- the remaining changes.
Thanks! r265852 fixed the code size regression for the user's project.
Why wasn't the test case (inline_unreachable-2.ll) checked in? The test passes only after the fix in r265852 is applied, so it seems that it could have been checked in too?
Submitted the second part. Instead of the third part, I think it is better to just return when Cost > Threshold instead of breaking out of the loop. Moving the check inside if (SingleBB && TI->getNumSuccessors() > 1) reduces the readability IMO.
I don't think it matters. AFAICT, the only situation where we use CA.Threshold even if CA.analyzeCall returns false is in providing diagnostics (-Rpass-analysis=inline and others). It'll still show Cost > Threshold. If we readjust, it might show Cost is much greater than threshold if the bonuses were removed, but in any case Cost itself is not accurate when we bail out early.
FYI, the second part I had already checked in has the same issue and I don't adjust the Threshold downwards.