Page MenuHomePhabricator

[Inliner] Optimize shouldBeDeferred
ClosedPublic

Authored by eraman on Jun 30 2017, 5:39 PM.

Details

Reviewers
chandlerc
Summary

This has some minor optimizations to shouldBeDeferred. This is not
strictly NFC because the early exit inside the loop assumes
TotalSecondaryCost is monotonically non-decreasing, which is not true if
the threshold used by CostAnalyzer is negative. AFAICT the thresholds do
not go below 0 for the default values of the various options we use.

Diff Detail

Event Timeline

eraman created this revision.Jun 30 2017, 5:39 PM
eraman added a subscriber: wmi.Jan 4 2019, 12:15 PM

Reviving this old patch as Wei recently noticed a case where a large function with many callees and many callers spent significant amount of time in shouldBeDeferred. This was triggered by change in profile causing many callees to be inlined into this large function.

chandlerc accepted this revision.Jan 4 2019, 1:13 PM

Nice, LGTM!

Does it make sense to add a (very early) assert that the thresholds are non-negative? I agree with you that it shouldn't be happening and it seems like a good invariant to enforce. Can of course be a follow-up patch.

lib/Transforms/IPO/Inliner.cpp
340

Despite the old variable's name, this should be ApplyLastCallBonus.

This revision is now accepted and ready to land.Jan 4 2019, 1:13 PM
eraman marked an inline comment as done.Jan 4 2019, 2:33 PM

Nice, LGTM!

Does it make sense to add a (very early) assert that the thresholds are non-negative? I agree with you that it shouldn't be happening and it seems like a good invariant to enforce. Can of course be a follow-up patch.

I don't know if we make implicit assumptions about threshold being positive, but I agree this seems a reasonable invariant to enforce. I turned on an assert and found that a test case that passes --inline-threshold=-2 breaks. I will add the assert in a follow-up patch along with the fix to the test.

eraman updated this revision to Diff 180327.Jan 4 2019, 2:34 PM

Fix variable naming.

eraman added a comment.Jan 4 2019, 4:30 PM

Reviving this old patch as Wei recently noticed a case where a large function with many callees and many callers spent significant amount of time in shouldBeDeferred. This was triggered by change in profile causing many callees to be inlined into this large function.

The instance Wei observed didn't have multiple callers. I mis-understood the case and Wei thought this patch fixed the issue, but it actually didn't. Still, I think this is a useful fix.

Reviving this old patch as Wei recently noticed a case where a large function with many callees and many callers spent significant amount of time in shouldBeDeferred. This was triggered by change in profile causing many callees to be inlined into this large function.

The instance Wei observed didn't have multiple callers. I mis-understood the case and Wei thought this patch fixed the issue, but it actually didn't. Still, I think this is a useful fix.

Totally agree, still LGTM.

eraman closed this revision.Jan 4 2019, 6:32 PM