Page MenuHomePhabricator

[AArch64] Fix incorrect MachinePointerInfo in splitStoreSplat

Authored by john.brawn on Feb 2 2017, 5:37 AM.



When splitting up one store into several in splitStoreSplat we have to make sure we get the MachinePointerInfo right, otherwise alias analysis thinks they all store to the same location. This can then cause invalid scheduling later on.

Diff Detail


Event Timeline

john.brawn created this revision.Feb 2 2017, 5:37 AM
gberry edited edge metadata.Feb 2 2017, 7:44 AM

Nice catch. Coincidentally I'm currently working on a patch that fixes a different bug related to this same transform.

Would you consider adding test cases that cover the other caller of splitStoreSplat, namely splitStores when it sees a store of a splat vector?

It also might be worth considering whether the tests should check the scheduler dependency debug output, since otherwise this bug is only exposed if the scheduler happens to pick a bad schedule.

Also, this seems like it probably should be considered for the release branch.

8926 ↗(On Diff #86793)

This should probably be const MachinePointerInfo &PtrInfo

3 ↗(On Diff #86793)

typo "generates" -> "generated"

11 ↗(On Diff #86793)

All of the CHECK-LABELS should probably include a ':' at the end to make the matching tighter:
e.g. CHECK-LABEL: test1:

john.brawn updated this revision to Diff 86972.Feb 3 2017, 8:46 AM

Update patch based on review comments.

hans added a subscriber: hans.Feb 3 2017, 2:03 PM

Also, this seems like it probably should be considered for the release branch.

cc'ing myself

gberry accepted this revision.Feb 6 2017, 8:47 AM

Looks good with a few minor nits.

13 ↗(On Diff #86972)

These first two checks can be CHECK-DAG since they can be scheduled independently. The same applies to all of these tests I believe.

10 ↗(On Diff #86972)

You might want to make the %vreg matching a little less brittle (i.e. match %vreg{{[0-9]+}} instead of %vreg3 since you aren't really checking anything related to the registers). The same probably goes for the Latency, which can probably just be dropped entirely. And the SU numbers.

This revision is now accepted and ready to land.Feb 6 2017, 8:47 AM
john.brawn added inline comments.Feb 6 2017, 9:48 AM
13 ↗(On Diff #86972)

Actually they can't be scheduled independently, they both write to x0+8 (the first writes xzr, the second writes x2).

10 ↗(On Diff #86972)

Sounds like a good idea, I'll do that.

gberry added inline comments.Feb 6 2017, 10:04 AM
13 ↗(On Diff #86972)

Ah yes, sorry. I misread the second store as an 'str'

This revision was automatically updated to reflect the committed changes.