Page MenuHomePhabricator

[ModuloSchedule] Add interface call to accept/reject SMS schedules
ClosedPublic

Authored by dpenry on Jun 30 2022, 1:29 PM.

Details

Summary

This interface allows a target to reject a proposed
SMS schedule. For Hexagon/PowerPC, all schedules
are accepted, leaving behavior unchanged. For ARM,
schedules which exceed register pressure limits are
rejected.

Also, two RegisterPressureTracker methods now need to be public so
that register pressure can be computed by more callers.

Diff Detail

Unit TestsFailed

TimeTest
60,100 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::scariness_score_test.cpp
Script: -- : 'RUN: at line 4'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fsanitize=address -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/asan/TestCases/scariness_score_test.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/asan/X86_64LinuxConfig/TestCases/Output/scariness_score_test.cpp.tmp

Event Timeline

dpenry created this revision.Jun 30 2022, 1:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 1:29 PM
dpenry requested review of this revision.Jun 30 2022, 1:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 1:29 PM

At a high level shouldUseSchedule sounds like a useful addition, but much of the logic in the current Arm implementation doesn't seem very Arm specific (correct me if I'm wrong). Would it be beneficial to incorporate the register pressure calculations into SwingSchedulerDAG, which the target could opt into checking.

llvm/include/llvm/CodeGen/TargetInstrInfo.h
738

"should be used"
Perhaps explain that returning false means no pipelined loops too. "Otherwise return false to not pipeline the loop."

llvm/lib/CodeGen/MachinePipeliner.cpp
2098

Perhaps move this below the "Schedule Found?" message below, and adding a debug message explaining that the target opted out of the schedule might be useful for debugging.

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
6842

cnt -> Cnt

6843

i -> I

6888

cycle -> Cycle. Same for the rest of the variables,

llvm/test/CodeGen/Thumb2/swp-regpressure.mir
7

This test uses phi nodes to increase the register pressure? Maybe remove the dead phis from dot2.

And maybe rename the tests to make it clear that dot1 has too high pressure, but dot2 is OK and expected to pipeilne.

dpenry marked 5 inline comments as done.Jul 18 2022, 3:26 PM

At a high level shouldUseSchedule sounds like a useful addition, but much of the logic in the current Arm implementation doesn't seem very Arm specific (correct me if I'm wrong). Would it be beneficial to incorporate the register pressure calculations into SwingSchedulerDAG, which the target could opt into checking.

The current logic isn't Arm-specific. This might be useful more generally, but I'd like to see more feedback before adding the calculations in.

llvm/test/CodeGen/Thumb2/swp-regpressure.mir
7
  • Renamed the tests, but the point of using the phi nodes in this way -- including the dead phis -- was to have the same set of instructions in each case with the only difference being the register pressure induced by them. Phi nodes are particularly nice because they don't affect RecMII.
dpenry updated this revision to Diff 445641.Jul 18 2022, 3:29 PM
  • Changes to comments
  • Debug message explaining that target opted out of schedule
  • Fixes to variable names
  • Naming of functions within test case
dmgreen accepted this revision.Jul 28 2022, 11:01 AM

OK. The logic looks OK to me, and we can always move the code if another target finds a use for it.
LGTM with a few suggestions

llvm/include/llvm/CodeGen/TargetInstrInfo.h
738

"should used" > "should be used"

llvm/lib/CodeGen/MachinePipeliner.cpp
2097

Remove this extra line.

llvm/lib/Target/Hexagon/HexagonInstrInfo.cpp
753

You could perhaps just make the default PipelinerLoopInfo::shouldUseSchedule return true to avoid these overrides.

This revision is now accepted and ready to land.Jul 28 2022, 11:01 AM
dpenry updated this revision to Diff 453152.Aug 16 2022, 4:15 PM
dpenry marked 3 inline comments as done.

Responding to comments

Comments addressed.

dmgreen accepted this revision.Aug 17 2022, 8:00 AM

LGTM