This is an archive of the discontinued LLVM Phabricator instance.

[MachineScheduler] checkResourceLimit boundary condition update
ClosedPublic

Authored by jsji on May 23 2019, 2:15 PM.

Details

Summary

When we call checkResourceLimit in bumpCycle or bumpNode, and we
know the resource count has just reached the limit (the equations
are equal). We should return true to mark that we are resource
limited for next schedule, or else we might continue to schedule
in favor of latency for 1 more schedule and create a schedule that
actually overbook the resource.

When we call checkResourceLimit to estimate the resource limit before
scheduling, we don't need to return true even if the equations are
equal, as it shouldn't limit the schedule for it .

Diff Detail

Repository
rL LLVM

Event Timeline

jsji created this revision.May 23 2019, 2:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 2:15 PM
Herald added a subscriber: hiraditya. · View Herald Transcript

Would this end up prioritising 'resourcelimited' too much? Looks like there can be performance differences between before and after this change - any numbers / thoughts on that?

llvm/lib/CodeGen/MachineScheduler.cpp
1841 ↗(On Diff #201063)

maybe reword 'equations are equal' to something more contextual ....

1845 ↗(On Diff #201063)

repeated computation

jsji marked 2 inline comments as done.EditedMay 24 2019, 6:20 AM

Would this end up prioritising 'resourcelimited' too much? Looks like there can be performance differences between before and after this change - any numbers / thoughts on that?

I think we just avoid disregarding 'resourcelimited' at certain situation, shouldn't be prioritising it too much.
I am working on gathering some performance data .

llvm/lib/CodeGen/MachineScheduler.cpp
1841 ↗(On Diff #201063)

Sure. How about "we just reach the resource limit"? Or any suggestion?

1845 ↗(On Diff #201063)

Thought build compiler should be able to optimize it. But yes, I can do it something like:

int ResCntFactor = (int)(Count - (Latency * LFactor));
if (AfterSchedNode)
    return ResCntFactor >= (int)LFactor;
else
    return ResCntFactor > (int)LFactor;
jsji updated this revision to Diff 202051.May 29 2019, 1:58 PM

Fix comment & repeated computation. Performance run still ongoing.

jsji added a comment.May 30 2019, 7:32 PM

Would this end up prioritising 'resourcelimited' too much? Looks like there can be performance differences between before and after this change - any numbers / thoughts on that?

The SPEC2k17 performance test on P9 system shows GEOMEAN 0.3% improvement,
with 1 biggest deg -0.50%, 2 biggest improvement +1.64%, +1.52%, others are within +-0.4%.

@javed.absar

jsji added a comment.Jun 6 2019, 7:15 PM

Ping.. Any more comments and feedback @javed.absar . Thanks.

javed.absar accepted this revision.Jun 7 2019, 1:47 AM

LGTM. Sorry for the delay

This revision is now accepted and ready to land.Jun 7 2019, 1:47 AM
jsji updated this revision to Diff 203556.Jun 7 2019, 7:42 AM

Thanks Javed! Rebased to ToT before committing.

This revision was automatically updated to reflect the committed changes.