Page MenuHomePhabricator

Cleanup Store Merging in UseAA case
ClosedPublic

Authored by niravd on Apr 8 2016, 2:41 PM.

Details

Summary

This patch fixes a bug (PR26827) when using anti-aliasing in store merging.
This sets the chain users of the component stores to point to the new merged store
instead of the component stores chain parent.

Diff Detail

Repository
rL LLVM

Event Timeline

niravd updated this revision to Diff 53090.Apr 8 2016, 2:41 PM
niravd retitled this revision from to Cleanup Store Merging in UseAA case.
niravd updated this object.
niravd added a reviewer: jyknight.
niravd added a subscriber: llvm-commits.
mgrang added a subscriber: mgrang.Apr 8 2016, 2:56 PM

Can you add a unit test for this?

Can you add a unit test for this?

Not one that highlights the difference. This is a factoring of some of the factorable cleanup of D14834. D14834 will remove the old path for store merging.

jyknight edited edge metadata.Apr 11 2016, 6:46 AM

Not one that highlights the difference. This is a factoring of some of the factorable cleanup of D14834. D14834 will remove the old path for store merging.

You ought to be able to detect that the chain is being set correctly now and wasn't before -- at the very least by looking at the nodes being produced.

Does this change by itself fix the test case in http://reviews.llvm.org/D17836?

Not one that highlights the difference. This is a factoring of some of the factorable cleanup of D14834. D14834 will remove the old path for store merging.

You ought to be able to detect that the chain is being set correctly now and wasn't before -- at the very least by looking at the nodes being produced.

Does this change by itself fix the test case in http://reviews.llvm.org/D17836?

Ah. So this isn't just a clean up, but actually fixing the UseAA case of merging. You're intuition is right here James, the test from D17836 case does work. I'll update the diff.

niravd updated this revision to Diff 53237.Apr 11 2016, 7:27 AM
niravd edited edge metadata.

Add test case

niravd updated this object.Apr 11 2016, 7:30 AM
mcrosier added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11232 ↗(On Diff #53237)

Please add a period.

Added cc's from D17836 which this obsoletes.

jyknight accepted this revision.Apr 12 2016, 2:25 PM
jyknight edited edge metadata.

LGTM (after fixing period as per other comment, of course).

This revision is now accepted and ready to land.Apr 12 2016, 2:25 PM
niravd updated this revision to Diff 53576.Apr 13 2016, 9:13 AM
niravd marked an inline comment as done.
niravd updated this object.
niravd edited edge metadata.

Add test case and period

This revision was automatically updated to reflect the committed changes.