Page MenuHomePhabricator

Fix two issues in MergeConsecutiveStores:

Authored by jyknight on Oct 27 2015, 8:24 PM.


  1. PR25154. This is basically a repeat of PR18102, which was fixed in

r200201, and broken again by r234430. The latter changed which of the
store nodes was merged into from the first to the last. Thus, we now
also need to prefer merging a later store at a given address into the
target node, instead of an earlier one.

  1. While investigating that, I also realized I'd introduced a bug in

r236850. There, I removed a check for alignment -- not realizing that
nothing except the alignment check was ensuring that none of the stores
were overlapping! This is a really bogus way to ensure there's no
aliased stores.

A better solution to both of these issues is likely to always use the
code added in the 'if (UseAA)' branches which rearrange the chain based
on a more principled analysis. I'll look into whether that can be used
always, but in the interest of getting things back to working, I think a
minimal change makes sense.

Diff Detail

Event Timeline

jyknight updated this revision to Diff 38625.Oct 27 2015, 8:24 PM
jyknight retitled this revision from to Fix two issues in MergeConsecutiveStores:.
jyknight updated this object.
jyknight added reviewers: spatel, majnemer.
jyknight added a subscriber: llvm-commits.

Ping; I'd really like to get this in ASAP since it fixes a miscompile regression.

hfinkel accepted this revision.Nov 1 2015, 11:36 PM
hfinkel added a reviewer: hfinkel.
hfinkel added a subscriber: hfinkel.


FWIW, I also recall thinking that we should be able to use the UseAA code here even when AA is not being used.

This revision is now accepted and ready to land.Nov 1 2015, 11:36 PM
arsenm added a subscriber: arsenm.Nov 1 2015, 11:45 PM
arsenm added inline comments.

This should use getStoreSize instead of * 8

jyknight closed this revision.Nov 2 2015, 10:52 AM
jyknight marked an inline comment as done.

Committed with rL251816 (forgot to add url to commit message).