This is an archive of the discontinued LLVM Phabricator instance.

Fix MemoryDependenceAnalysis in cases where QueryInstr is a CmpXchg or a AtomicRMW
ClosedPublic

Authored by morisset on Aug 21 2014, 6:25 PM.

Details

Summary

MemoryDependenceAnalysis is currently cautious when the QueryInstr is an atomic
load or store, but I forgot to check for atomic cmpxchg/atomicrmw. This patch
is a way of fixing that, and making it less brittle (i.e. no risk that I forget
another possible kind of atomic, even if the IR ends up changing in the future),
by adding a fallback checking mayReadOrWriteFromMemory.

Thanks to Philip Reames for finding this bug and suggesting this solution in
http://reviews.llvm.org/D4845

Sadly, I don't see how to add a test for this, since the passes depending on
MemoryDependenceAnalysis won't trigger for an atomic rmw anyway. Does anyone
see a way for testing it?

Diff Detail

Event Timeline

morisset updated this revision to Diff 12823.Aug 21 2014, 6:25 PM
morisset retitled this revision from to Fix MemoryDependenceAnalysis in cases where QueryInstr is a CmpXchg or a AtomicRMW.
morisset updated this object.
morisset edited the test plan for this revision. (Show Details)
morisset added reviewers: jfb, reames.
morisset added a subscriber: Unknown Object (MLST).
morisset updated this revision to Diff 13109.Aug 29 2014, 2:31 PM

Rebasing on trunk.

reames edited edge metadata.Aug 29 2014, 5:14 PM

LGTM

lib/Analysis/MemoryDependenceAnalysis.cpp
426

nitpick: } else if (c) {

morisset updated this revision to Diff 13120.Aug 29 2014, 5:29 PM
morisset edited edge metadata.

Fix the formatting

Can you accept the revision if it looks good to you ?
Thanks.

jfb accepted this revision.Aug 30 2014, 9:27 AM
jfb edited edge metadata.
This revision is now accepted and ready to land.Aug 30 2014, 9:27 AM
jfb added a comment.Aug 30 2014, 9:27 AM

lgtm+accept

morisset closed this revision.Sep 2 2014, 1:27 PM
morisset updated this revision to Diff 13179.

Closed by commit rL216940 (authored by @morisset).