Page MenuHomePhabricator

Cleanup early-exit from analyzeCall
Needs ReviewPublic

Authored by eraman on Mar 11 2015, 2:19 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

This makes the following changes to the early bailout code:

  1. Speculatively apply all possible bonuses to Threshold initially. Whenever we know a certain bonus can not be applied, subtract it from Threshold. Note that we are still not processing the entire function, although we might delay early bailout.
  2. It currently calculates the 10% and 50% vector bonuses after applying a single basic-block bonus on the original threshold. I think expressing the bonuses in terms of the original threshold makes it less confusing.
  3. Set the threshold to 0 instead of 1 when an unreachable instruction is seen after the call. Has no practical effect, but keeps it in line with the comment before that code.

Diff Detail

Repository
rL LLVM

Event Timeline

eraman updated this revision to Diff 21758.Mar 11 2015, 2:19 PM
eraman retitled this revision from to Cleanup early-exit from analyzeCall.
eraman updated this object.
eraman edited the test plan for this revision. (Show Details)
eraman set the repository for this revision to rL LLVM.
eraman added a subscriber: Unknown Object (MLST).

I think it would even more clear to reverse everything.

Add all of the possible bonuses to the threshold, and when they cease to apply, subtract them, including subtracting any part of the vector bonus that ended up not applying at the very end.

That makes it more clear that this doesn't meaningfully change the bounds on how much of the function we look at, and there aren't two thresholds at all. We just need to have the threshold always be the max of all possible bonuses which could apply.

(It would be good to update the commit log message as well to be more clear that this doesn't cause us to look at the entire function, just to assume the maximum bonuses will apply until proven wrong.)

eraman updated this revision to Diff 21783.Mar 11 2015, 4:05 PM
eraman updated this object.

Based on Chandler's comments, removed MaxThreshold, speculatively bumped up Threshold to include maximum possible vector bonus and readjust the threshold when the actual vector bonus could be determined.

chandlerc edited edge metadata.Mar 13 2015, 2:05 PM

(sorry for delays)

lib/Analysis/IPA/InlineCost.cpp
1229–1240

How about just moving the comment up above NumVectorInstructions checks, and do two different subtracts from Threshold based on the specific instruction ratios?

1237

Where do you subtract off the single-bb bonus?

test/Transforms/Inline/vector-bonus.ll
2

Did you try running this test without your change? I think you will find that it doesn't fail -- you don't pipe the output of opt to the FileCheck tool, and so we never check anything.

I would suggest "testing your test" by ensuring your test fails with the old version of opt first.

36–38

I would use CHECK-LABEL and put the checks inside the function body of @bar so its clear that there are checks associated with that function.

Sorry about the bad testcase. I verified that inlining didn't happen before and happened after the change by printing debug messages, but never tested the test case as written. I will update it.

lib/Analysis/IPA/InlineCost.cpp
1229–1240

If you mean something like
if (NumVectorInstructions <= NumInstructions / 10)

  Threshold -= FiftyPercentVectorBonus;
else if (NumVectorInstructions <= NumInstructions / 2)
  Threshold -= (FiftyPercentVectorBonus - TenPercentVectorBonus);

a side effect of the above is that we won't set VectorBonus and won't print the value as part of the DEBUG_PRINT_STAT. Not a big deal, but I've found the stats useful. (Or do you want to move the Threshold += VectorBonus to where VectorBonus is set?)

eraman updated this revision to Diff 22032.Mar 16 2015, 10:34 AM
eraman edited edge metadata.

Fixed the test case and changed the code that computed the revised threshold based on review comments.