This is an archive of the discontinued LLVM Phabricator instance.

Refactor alias check from MISched into common helper. NFC.
ClosedPublic

Authored by efriedma on Mar 3 2017, 5:41 PM.

Details

Summary

The API seems okay as-is, but maybe it could be improved a little. Maybe it makes sense to loosen the mayStore requirement?

Diff Detail

Event Timeline

efriedma created this revision.Mar 3 2017, 5:41 PM
MatzeB added a comment.Mar 3 2017, 6:02 PM

One thing I found kind of confusing was that I'm not sure what the rules for areMemAccessesTriviallyDisjoint are supposed to be. It looks like a lot of targets assume that the function is in SSA form (checking equality between two registers without checking whether the value of that register is modified), but we call it after register allocation. How is this supposed to work?

I'm no expert here but I think the areMemAccessesTriviallyDisjoint() works because it is only used for scheduling where a change in the value of the register would inhibit reordering anyway.

MatzeB added inline comments.Mar 3 2017, 6:06 PM
lib/CodeGen/MachineInstr.cpp
1574

Use a reference for Other as that must not be a nullptr.

mcrosier edited edge metadata.Mar 6 2017, 10:10 AM
mcrosier added a subscriber: gberry.

One thing I found kind of confusing was that I'm not sure what the rules for areMemAccessesTriviallyDisjoint are supposed to be. It looks like a lot of targets assume that the function is in SSA form (checking equality between two registers without checking whether the value of that register is modified), but we call it after register allocation. How is this supposed to work?

I'm no expert here but I think the areMemAccessesTriviallyDisjoint() works because it is only used for scheduling where a change in the value of the register would inhibit reordering anyway.

Yes, I believe this is the same conclusion @gberry and I arrived at in the recent past.

efriedma updated this revision to Diff 91077.Mar 8 2017, 2:43 PM
efriedma edited the summary of this revision. (Show Details)

Make mayAlias() take a reference to Other. Update comments.

mcrosier accepted this revision.Mar 9 2017, 7:02 AM
mcrosier added inline comments.
lib/CodeGen/MachineInstr.cpp
1579

I think it makes sense to relax this constraint, as you suggest Eli.

This revision is now accepted and ready to land.Mar 9 2017, 7:02 AM
This revision was automatically updated to reflect the committed changes.