[PATCH] Relax atomic restrictions on memory dependence analysis
Philip Reames
listmail at philipreames.com
Mon Aug 18 12:12:27 PDT 2014
This version looks close to ready. See inline comments. Once you fix those, I'll give one more read through before officially giving an LGTM.
================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:414
@@ -412,3 +413,3 @@
// FIXME: This is overly conservative.
- if (!LI->isUnordered())
- return MemDepResult::getClobber(LI);
+ if (!LI->isUnordered()) {
+ if (!QueryInst || LI->getOrdering() != Monotonic)
----------------
I originally wrote: "You're still missing the point of my previous example. The problem is that ordered operations have additional *aliasing* requirements. Your code is still not checking for these conditions and thus is still incorrect. "
I believe your code is correct. You're making a fairly subtle point with your checks though. Any two potentially aliased monotonic loads are ordered, but a monotonic and unordered load are not. Even if they alias. Please clarify this in comments. I nearly missed it (as you can see from the comment I left in above.)
I would suggest that you change the aliasing specific comment in the same loop. Keep the comments in sync with the code.
Nitpick: Separate the first "if (!QueryInst || LI->getOrdering() != Monotonic)" into two clauses. Semantically, the checks are unrelated.
================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:423
@@ -415,1 +422,3 @@
+ return MemDepResult::getClobber(LI);
+ }
----------------
You need to check if LI isVolatile. LoadInst::isUnordered does this, and you only want to change memory ordering here. I'd suggest having that be it's own top most check for clarity.
================
Comment at: test/Transforms/DeadStoreElimination/atomic.ll:131
@@ +130,3 @@
+}
+
+; DSE across monotonic load (forbidden since the eliminated store is atomic)
----------------
You could use a couple of other test cases here. In particular, load-value forwarding across monotonic loads and stores would be helpful.
A positive example:
store x = 0
store y monotonic
load x <- can use 0 since doesn't participate in ordering even if x == y at runtime
A negative example:
store x = 0 monotonic
load y monotonic
load x monotonic <-- not safe to forward!
And an ambiguous example:
store x = 0 unordered
load y monotonic
load x monotonic <-- is this correct to forward? I don't know.
http://reviews.llvm.org/D4797
More information about the llvm-commits
mailing list