Page MenuHomePhabricator

[MachinePipeliner] Fix a bug in Output Dependency chains
ClosedPublic

Authored by lsaba on Mar 1 2020, 7:23 AM.

Details

Summary

The current implementation collects all Preds/Succs of a Dep of kind Output, creating a long chain and subsequently a schedule with an unnecessarily large II.
Was this done on purpose for a reason I'm missing?

Diff Detail

Event Timeline

lsaba created this revision.Mar 1 2020, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2020, 7:23 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
bcahoon accepted this revision.EditedMar 4 2020, 8:13 AM

Thanks for the patch! Your suggested change is correct. The original code only "worked" because output dependences are rare. But, I can see that if the DAG is more complicated, and there is a mix of output and order dependences, that the original code would cause problems when computing the MII. If you have a test case, please add one.

Thanks,
Brendon

This revision is now accepted and ready to land.Mar 4 2020, 8:13 AM
lsaba added a comment.Mar 24 2020, 7:28 AM

I can't seem to get commit access to GitHub (already emailed llvm-admin twice)
Would anyone be kind enough to commit the change on behalf of me? Thanks!

jsji added a comment.Mar 24 2020, 7:30 AM

I will commit it for you.

lsaba added a comment.Mar 24 2020, 7:32 AM

I will commit it for you.

Thanks!

This revision was automatically updated to reflect the committed changes.