This is an archive of the discontinued LLVM Phabricator instance.

Use a much more efficient (linear instead of quadratic?) algorithm for computing the height: standard DFS mincost algorithm for a DAG.
AcceptedPublic

Authored by chandlerc on Jun 16 2016, 4:05 AM.

Details

Reviewers
atrick
Summary

Not only is the old algorithm essentially BFS instead of DFS, it weirdly
can mark heights as dirty which then walks preds. Very strang. I think
a direct DFS that just computes the height of everything in the DAG from
the bottom up and avoids re-computing already computed heights is much
easier to understand.

For test/CodeGen/AMDGPU/spill-scavenge-offset.ll, prior to it having any
scheduler turned off, computing the height was over 25% of the runtime.
With this patch, it is completely gone. I'm measuring improvements from
32s to 24s (25%) in debug builds and 4.5s to 3.17s (30%) in an optimized
build for thes test.

Glancing at it, the depth computation probably needs the same treatment,
but I've not yet found a test that exercises this (I've not looked too
hard yet though).

As with my prior patch, this impacts both SDAG scheduling and MI
scheduling, but for different reasons -- in this case, both schedulers
call the ScheduleDAG's getHeight routine heavily and were causing it
show up in profiles for spill-scavenge-offset.ll.

Diff Detail

Event Timeline

chandlerc updated this revision to Diff 60961.Jun 16 2016, 4:05 AM
chandlerc retitled this revision from to Use a much more efficient (linear instead of quadratic?) algorithm for computing the height: standard DFS mincost algorithm for a DAG..
chandlerc updated this object.
chandlerc added a subscriber: llvm-commits.
MatzeB added a subscriber: atrick.Jun 16 2016, 3:50 PM
atrick accepted this revision.Jun 16 2016, 9:45 PM
atrick added a reviewer: atrick.

Awesome. Proper DFS is how I've done it in the past.

ComputeDepth should be rewritten too.

This revision is now accepted and ready to land.Jun 16 2016, 9:45 PM

Look like patch was not committed.