Page MenuHomePhabricator

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

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



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


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

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
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
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


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
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
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
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.