Page MenuHomePhabricator

AMDGPU: Partial ILP scheduler port from SelectionDAG to SchedulingDAG (experimental)
ClosedPublic

Authored by vpykhtin on Nov 10 2017, 5:09 AM.

Details

Summary

This is a partial port in that register pressure lowering checks aren't yet done because it isn't really straitforward to port them from the old place

There're some comments left copypasted from the old place, it may be outdated/irrelevant.

Diff Detail

Repository
rL LLVM

Event Timeline

vpykhtin created this revision.Nov 10 2017, 5:09 AM
rampitec edited edge metadata.Nov 10 2017, 12:22 PM

Can you add some tests just to show it does not crash? Maybe add run-lines to schedule-regpressure-limit.ll, schedule-regpressure-limit2.ll

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
224 ↗(On Diff #122419)

Can we use ld/st clustering mutations and MacroFusion to it?

rampitec added inline comments.Nov 10 2017, 12:30 PM
lib/Target/AMDGPU/GCNILPSched.cpp
243 ↗(On Diff #122419)

Can queue be empty?

283 ↗(On Diff #122419)

spaces around ==.

vpykhtin updated this revision to Diff 122649.Nov 13 2017, 7:18 AM

fixed per review issues.

vpykhtin marked 3 inline comments as done.Nov 13 2017, 7:19 AM
rampitec added inline comments.Nov 13 2017, 9:17 AM
lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
224 ↗(On Diff #122419)

i think we need MacroFusion as well.

vpykhtin updated this revision to Diff 123341.Nov 17 2017, 7:33 AM
vpykhtin edited the summary of this revision. (Show Details)

Added test. It turns out getOccupancyWithLocalMemSize function doesn't account for amdgpu-waves-per-eu attribute so I added call to getWavesPerEU (which does).

rampitec accepted this revision.Nov 17 2017, 8:50 AM

LGTM

lib/Target/AMDGPU/GCNIterativeScheduler.cpp
569 ↗(On Diff #123341)

Looks like this call is missing in the GCNSchedStrategy as well, so needs to be added there as well.

This revision is now accepted and ready to land.Nov 17 2017, 8:50 AM
vpykhtin added inline comments.Nov 17 2017, 8:54 AM
lib/Target/AMDGPU/GCNIterativeScheduler.cpp
569 ↗(On Diff #123341)

Right, but I wonder if this should be added into getOccupancyWithLocalMemSize itself? It uses getMaxWavesPerEU which probably should be changed to getWavesPerEU.

rampitec added inline comments.Nov 17 2017, 9:42 AM
lib/Target/AMDGPU/GCNIterativeScheduler.cpp
569 ↗(On Diff #123341)

It probably should, though I believe it should go as a separate change.

arsenm added inline comments.Nov 17 2017, 4:46 PM
test/CodeGen/AMDGPU/schedule-ilp.ll
3 ↗(On Diff #123341)

This isn't a useful check. Is there something more specific to check, like there are no spills or some other improvement over the default?

This revision was automatically updated to reflect the committed changes.