Page MenuHomePhabricator

[MachineScheduler] Clear subregister entries also in addPhysRegDeps when handling a def
ClosedPublic

Authored by jonpa on May 14 2018, 10:39 AM.

Details

Reviewers
atrick
MatzeB
Summary

When working with the SystemZ InstRWs, I found it disturbing to find edges that were seemingly unnecessary in the DAGs.

To my surprise I found that when going bottom-up while building the schedgraph, the uses/defs entries for subregs of Reg were not removed. I was expecting this since those earlier subregs cannot use a later seen (bottom-up) def of that subreg, due to the current def of Reg. Is there a reason that this is not done?

My patch is very nearly NFC on SystemZ benchmarks. In fact, just 2 or 4 instructions on SPEC move around, so it's not strictly NFC, but... I think that case was due to some height difference that resulted from such an edge for a subreg around the Reg SU. Of course, if SU(0) defines Reg:sub, SU(1) defines Reg, and SU(2) uses Reg:Sub, there is no use to have an edge from SU(0) to SU(2) for Reg:sub, since Reg already forces edges from SU(0) -> SU(1) and SU(1) -> SU(2).

It seems like a good idea to minimize the number of DAG edges if there is no particular reason to have them, for performance reasons. It also makes the DAG easier to read and handle.

It could be that I am missing something here, for instance is a subreg always completely covered by Reg? In that case this patch only makes sense on targets that don't have that property, unfortunately.

Diff Detail

Event Timeline

jonpa created this revision.May 14 2018, 10:39 AM

We didn't support cross-call scheduling last I looked at this. Hopefully @MatzeB can verify that your fix is correct.

jonpa added a comment.May 15 2018, 3:52 AM

We didn't support cross-call scheduling last I looked at this. Hopefully @MatzeB can verify that your fix is correct.

I did not intend to change anything in regards to the handling of calls, just to also handle subregs with Uses and Defs. Did I miss something?

Is there some other user of ScheduleDAGInstrs::buildSchedGraph() than MachineScheduler that allows rescheduling around calls, and that's why the handling of calls is here?

atrick accepted this revision.May 23 2018, 1:04 PM

LGTM, ignore my previous comment and sorry for the delay.

This revision is now accepted and ready to land.May 23 2018, 1:04 PM
jonpa closed this revision.May 24 2018, 1:43 AM

Committed as r333165.