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

[David Majnemer via llvm-commits]

majnemer added a subscriber: majnemer.
majnemer added a comment.

This LGTM but another pair of eyes wouldn't hurt.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:285
@@ -285,1 +284,3 @@
+  /// A scoped hash table of the current values of previously encounted memory
+  /// locations
   ///
----------------
Please end this with a period.

================
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,
----------------
You do not initialize IsAtomic in the default constructor.  It might be nicer to use the NSDMI-style to make it stand out more.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:610
@@ +609,3 @@
+      // (conservatively) we can't peak past the ordering implied by this
+      // operation, but we can add this load to our set of available values
+      if (MemInst.isVolatile() || !MemInst.isUnordered()) {
----------------
Please end this sentence with a period.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:621
@@ +620,3 @@
+          InVal.MatchingId == MemInst.getMatchingId() &&
+          // We don't yet handle removing loads with ordering of any kind
+          !MemInst.isVolatile() && MemInst.isUnordered() &&
----------------
Please end this sentence with a period.

================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:727
@@ -673,1 +726,3 @@
+        // favor of a later one which isn't.  We could in principal remove an
+        // earlier unordered store if the later one is also unordered
         if (MemInst.isSimple())
----------------
Please end this with a period.

================
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
+}
----------------
Instead of relying on the calls to SimplifyInstruction to wipe away the sub, why not just `ret %b` and check for `ret i32 %a` ?


http://reviews.llvm.org/D15337