This is an archive of the discontinued LLVM Phabricator instance.

[DAG] Improve store merge candidate pruning.
ClosedPublic

Authored by niravd on Apr 14 2017, 8:16 AM.

Details

Summary

Remove non-consecutive stores from candidate search as they can never
be merged and may prevent us from finding subsequent merges.

Event Timeline

niravd created this revision.Apr 14 2017, 8:16 AM
spatel edited edge metadata.Apr 17 2017, 6:54 AM

The limitations of store merging aren't clear to me. Can you add a code comment about why this is necessary? I would have assumed that we'd merge the later consecutive stores when visiting them because we visited them first.
Also, it would be better to add a more obvious test case. I think this should work as an x86 test:

; Don't let a non-consecutive store thwart merging of the last two.
define void @almost_consecutive_stores(i8* %p) {
store i8 0, i8* %p
%p1 = getelementptr i8, i8* %p, i64 42
store i8 1, i8* %p1
%p2 = getelementptr i8, i8* %p, i64 2
store i8 2, i8* %p2
%p3 = getelementptr i8, i8* %p, i64 3
store i8 3, i8* %p3
ret void
}

If that's correct, can you add that test as an NFC change ahead of this patch, so we just see the diff from your patch? Thanks.

niravd updated this revision to Diff 95564.Apr 18 2017, 7:27 AM

Update with Sanjay's suggestions and rebased with to show additional testcase

spatel accepted this revision.Apr 18 2017, 8:38 AM

LGTM, but see inline for nits in code comments.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12380

comma after "However"

12382

mismatched (}
comma before "we"

12383

typo: mergeable

12392

we've -> we
Candidates -> candidates
to do a -> to

This revision is now accepted and ready to land.Apr 18 2017, 8:38 AM
This revision was automatically updated to reflect the committed changes.