This is an archive of the discontinued LLVM Phabricator instance.

[DAGCombine] Fix bug in MergeConsecutiveStores
ClosedPublic

Authored by ahatanak on Apr 6 2015, 4:52 PM.

Details

Summary

This bug manifests when there are two loads and two stores that are chained as follows in a DAG,

(ld v3f32) -> (st f32) -> (ld v3f32) -> (st f32)

and the store's values are extracted from the preceding vector loads.

The current code in trunk creates a build_vector node and then inserts the new merged store node between the two load nodes. This creates a cycle between the merged store node, the build_vector node, the extract_vector_elt node, and the second vector load (set a breakpoint at DAGCombiner.cpp:10056 to visualize the transformation), which eventually results in a crash during type legalization.

This patch fixes the bug by inserting the new merged store at the position of the last store node in the chain.

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 23301.Apr 6 2015, 4:52 PM
ahatanak retitled this revision from to [DAGCombine] Fix bug in MergeConsecutiveStores.
ahatanak updated this object.
ahatanak edited the test plan for this revision. (Show Details)
ahatanak added reviewers: spatel, nadav.
ahatanak added a subscriber: Unknown Object (MLST).
spatel edited edge metadata.Apr 7 2015, 11:14 AM

Hi Akira,

I don't feel qualified to fully review this patch; it's not clear to me that choosing the last store is the correct solution...
so I've just pointed out some nits. :)

I'm curious if you tracked down why this fails on aarch64 but not x86-64? Something in the x86 backend prevents the stores from merging?

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
10064–10066

Comment does not match code now: "earliest store" should be "latest store"?

10134–10135

Comment does not match code now: "first store" should be "last store"?

10522–10524

Comment does not match code now: "earliest store" should be "latest store"?

10567–10568

Comment does not match code now: "first store" should be "last store"?

test/CodeGen/AArch64/merge-store.ll
7

Can the test case be minimized? Do we need @g1?
I think it would be good to also add check lines to makes sure that the loads were merged into the register as expected ahead of the store.

It's not clear to me that choosing the last store is the correct solution...

We already check memory aliasing before calling MergeStoresOfConstantsOrVecElts, so I think only direct data dependence matters; but stores to not produce any (non-chain) values so moving them later can't create a loop.

So I think the patch is essentially sound.

Tim.

ahatanak updated this revision to Diff 23375.Apr 7 2015, 3:34 PM
ahatanak edited edge metadata.

When the target is x86-64, DAGCombiner::MergeStoresOfConstantsOrVecElts returns early because v2f32 is not a legal vector type for x86-64.

I've updated the comments and simplified the test case a little as per Sanjay's request.

Tim & Sanjay, does this look OK?

spatel accepted this revision.Apr 8 2015, 11:25 AM
spatel edited edge metadata.

With Tim's explanation, I think this is fine. LGTM.

This revision is now accepted and ready to land.Apr 8 2015, 11:25 AM

Thanks, I'll commit the patch today.

This revision was automatically updated to reflect the committed changes.