Page MenuHomePhabricator

[ScheduleDAGInstrs] Handle instructions with multiple MMOs
ClosedPublic

Authored by gberry on Mar 11 2016, 11:39 AM.

Details

Summary

In getUnderlyingObjectsForInstr(): Don't give up on instructions with
multiple MMOs, instead look through all the MMOs and if they all meet
the conservative criteria previously used for single MMO instructions,
then return all of the underlying objects derived from the MMOs.

The change to ScheduleDAGInstrs::buildSchedGraph() is needed to avoid
the case where multiple underlying objects are present and are related
in such a way that successive iterations of the loop end up adding a
dependency from an instruction to itself.

Diff Detail

Repository
rL LLVM

Event Timeline

gberry updated this revision to Diff 50455.Mar 11 2016, 11:39 AM
gberry retitled this revision from to [ScheduleDAGInstrs] Handle instructions with multiple MMOs.
gberry updated this object.
gberry added reviewers: atrick, hfinkel.
gberry added a subscriber: llvm-commits.

Seems reasonable to me, but I think Hal/Andy or someone else more familiar with this could should approve the patch.

lib/CodeGen/ScheduleDAGInstrs.cpp
1048 ↗(On Diff #50455)

underlObj should be capitalized.

gberry added inline comments.Mar 23 2016, 9:24 AM
lib/CodeGen/ScheduleDAGInstrs.cpp
1048 ↗(On Diff #50455)

This is a copy of the loop above. I think the capitalization of this (and stores_, which should probably also drop the '_') should be a separate change.

mcrosier added inline comments.Mar 23 2016, 9:38 AM
lib/CodeGen/ScheduleDAGInstrs.cpp
1048 ↗(On Diff #50455)

Fair enough.

I also think this is reasonable patch.

atrick accepted this revision.Apr 11 2016, 1:20 PM
atrick edited edge metadata.

This looks fine to me. Thank you, and sorry for not reviewing it earlier.

This revision is now accepted and ready to land.Apr 11 2016, 1:20 PM

Some coding style suggestions:

lib/CodeGen/ScheduleDAGInstrs.cpp
162 ↗(On Diff #50455)

Please avoid auto in contexts where the type is not obvious! Using MachineMemOperand here is easy and if possible also add const.

Maybe factor all of the code inside the loop into an own function with bool return value and perform the Object.clear() cleanup when the function returned false?

1048 ↗(On Diff #50455)

Please void auto in contexts where the type is not obvious. An obvious example would be auto *X = new SomeType(); because the type can be seen on the right side, it is not obvious here what type the elements in Objs have.

@atrick No problem, thanks for the review.

@MatzeB I'll address your comments along with Chad's in a follow up NFC change.

This revision was automatically updated to reflect the committed changes.