[PATCH] D15337: [EarlyCSE] Value forwarding for unordered atomics

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:303
@@ -296,1 +302,3 @@
+    bool IsAtomic;
     LoadValue() : Data(nullptr), Generation(0), MatchingId(-1) {}
+    LoadValue(Value *Data, unsigned Generation, unsigned MatchingId,
----------------
majnemer wrote:
> You do not initialize IsAtomic in the default constructor.  It might be nicer to use the NSDMI-style to make it stand out more.
Ouch!  Good catch.  Thanks.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:624
@@ -578,1 +623,3 @@
+          // We can't replace an atomic load with one which isn't also atomic.
+          InVal.IsAtomic >= MemInst.isAtomic()) {
         Value *Op = getOrCreateResult(InVal.Data, Inst->getType());
----------------
dberlin wrote:
> At least at a glance, this looks like you compare a bool with a bool using >=, which is pretty confusing?
> 
That is what I'm doing.  This really calls for an implication operator, but C++ doesn't have one of those?  I could extract out a helper function I guess to give it a name.

================
Comment at: test/Transforms/EarlyCSE/atomics.ll:17-22
@@ +16,8 @@
+define i32 @test13(i1 %B, i32* %P1) {
+  %a = load atomic i32, i32* %P1 seq_cst, align 4
+  %b = load i32, i32* %P1
+  %res = sub i32 %a, %b
+  ret i32 %res
+  ; CHECK: load atomic i32, i32* %P1
+  ; CHECK: ret i32 0
+}
----------------
majnemer wrote:
> Instead of relying on the calls to SimplifyInstruction to wipe away the sub, why not just `ret %b` and check for `ret i32 %a` ?
Because then %a becomes dead and there's nothing preventing us from removing it instead of forwarding.


http://reviews.llvm.org/D15337