This is an archive of the discontinued LLVM Phabricator instance.

[Power9] Code Cleanup - Remove needsAggressiveScheduling()
ClosedPublic

Authored by stefanp on Jun 27 2018, 11:06 AM.

Details

Summary

As we already return true from needsAggressiveScheduling() for the most recent hardware it would be cleaner to just return true for all PowerPC hardware.

Three tests were deleted because they were looking for correct spill behavior however with the use of the machine scheduler the spill is not longer required and therefore cannot be checked for correctness

2011-12-05-NoSpillDupCR.ll
save-cr-ppc32svr4.ll
save-crbp-ppc32svr4.ll

Also, the following test ppc64-gep-opt.ll was cut down significantly because the removal of needsAggressiveScheduling() also means that PPCSubtarget::useAA() always returns true. That test checked with and without AA. I have removed the without AA portion of the test.

Diff Detail

Repository
rL LLVM

Event Timeline

stefanp created this revision.Jun 27 2018, 11:06 AM
echristo added inline comments.Jun 27 2018, 11:21 AM
lib/Target/PowerPC/PPCSubtarget.cpp
169 ↗(On Diff #153128)

Might want to fix up the comment? :)

189–190 ↗(On Diff #153128)

These appear to be the defaults?

198 ↗(On Diff #153128)

Comment update?

stefanp updated this revision to Diff 153164.Jun 27 2018, 1:34 PM

Updated the comments.
Sorry.. I should have caught them initially.

For the defaults:

Policy.OnlyTopDown = false; 
Policy.OnlyBottomUp = false;

The flag Policy.OnlyTopDown does default to false.
However, GenericScheduler::initPolicy changes the default of Policy.OnlyBottomUp to true. I've fixed this so that I only use Policy.OnlyBottomUp = false; and not the other flag which is already a default.

echristo added inline comments.Jun 27 2018, 2:52 PM
lib/Target/PowerPC/PPCSubtarget.cpp
169 ↗(On Diff #153128)

I'd probably just delete the comment.

189–190 ↗(On Diff #153128)

We should probably comment on why we do this?

198 ↗(On Diff #153128)

Same.

stefanp updated this revision to Diff 153369.Jun 28 2018, 12:33 PM

Fixed comments according to previous review.

Also, the three tests that I wanted to delete I've actually kept. I added asm to each of those three tests to force the code to spill the condition register. If the condition register is spilled the tests can once again check to see if the spill is correct.

Looking pretty good at this point. I am still curious why we've got the scheduling change here...

lib/Target/PowerPC/PPCSubtarget.cpp
189–190 ↗(On Diff #153128)

I do wonder "why" we want to do this rather than just that we're doing it.

Hi Eric,

We set the generic scheduler to bi-directional scheduling back in 2013. It was at the time when the generic machine scheduler was added to PowerPC. I can only assume that when we did this we ran a set of benchmarks and determined that this is the best option based on that.
However, what I can do now is run a round of SPEC with Top-Of-Trunk and confirm that we still do better in the bi-directional case. (Or at least not worse.)

Does that sound reasonable?

Hi Eric,

We set the generic scheduler to bi-directional scheduling back in 2013. It was at the time when the generic machine scheduler was added to PowerPC. I can only assume that when we did this we ran a set of benchmarks and determined that this is the best option based on that.
However, what I can do now is run a round of SPEC with Top-Of-Trunk and confirm that we still do better in the bi-directional case. (Or at least not worse.)

Yep. Those benchmarks were likely run on a P7, so rechecking is probably a good idea.

Does that sound reasonable?

Hi Hal, Eric,

I ran SPEC2017 on a P9 machine and difference between bi-directional and bottom up scheduling was very small overall. The bi-directional scheduling continues to be a bit better with
xalancbmk - 0.5% better on bi-directional
leela - 2.0% better on bi-directional

Changes for all of the other benchmarks were too small.

Hi Hal, Eric,

I ran SPEC2017 on a P9 machine and difference between bi-directional and bottom up scheduling was very small overall. The bi-directional scheduling continues to be a bit better with
xalancbmk - 0.5% better on bi-directional
leela - 2.0% better on bi-directional

Changes for all of the other benchmarks were too small.

Sounds good. Thanks for checking. I recommend keeping it bi-directional.

nemanjai accepted this revision.Jul 17 2018, 6:00 AM

LGTM.

lib/Target/PowerPC/PPCSubtarget.cpp
187 ↗(On Diff #153369)

I agree with Eric, please add the reason. Perhaps We want to do bi-directional scheduling since it provides a more balanced schedule leading to better performance.

This revision is now accepted and ready to land.Jul 17 2018, 6:00 AM
lib/Target/PowerPC/PPCSubtarget.cpp
187 ↗(On Diff #153369)

Yeah... though that's not even really a reason, just describing the effect. :\

This revision was automatically updated to reflect the committed changes.