[PATCH] D15592: [LICM] Make store promotion work in the face of unordered atomics

[JF Bastien via llvm-commits]

jfb added a comment.

Can you add tests which validates that:

- Atomics in a loop aren't moved past a signal fence.
- Volatile atomics aren't affected.

Also, parts of the C++ standards committee thinks that the standard should "discourage non-normatively aggressive optimizations, e.g across large or unbounded loops" in the context of p0062r0 <http://wg21.link/p0062r0>. There isn't full consensus on this yet, but I'd be cautious about doing anything too aggressive to `relaxed` (a.k.a. `monotonic`) accesses for now. It would be good to also add a test for the example in p0062r0 and make sure it doesn't get optimized for now. I'll ping Hans to see what he thinks.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:939
@@ -926,3 +938,3 @@
             GuaranteedToExecute = true;
             Alignment = InstAlignment;
           }
----------------
I'm being a bit paranoid here (since this shouldn't happen because you prevent mixing of atomic / non-atomic), but it would be good to `assert((Alignment == 0 ? !store->isAtomic() : true) && "atomic alignment can't be 0");` just to make sure that future edits don't introduce bugs.

================
Comment at: test/Transforms/LICM/atomics.ll:69
@@ -69,3 +68,3 @@
   %vala = load atomic i32, i32* %y monotonic, align 4
   store atomic i32 %vala, i32* %x unordered, align 4
   %exitcond = icmp ne i32 %vala, 0
----------------
Could you add a similar test where the loop does a  monotonic load followed by a monotonic store?

================
Comment at: test/Transforms/LICM/atomics.ll:77
@@ +76,3 @@
+; CHECK-LABEL: loop:
+; CHECK: load atomic i32, i32* %y monotonic
+; CHECK-LABEL: end:
----------------
`CHECK-NOT: store`

================
Comment at: test/Transforms/LICM/atomics.ll:102
@@ +101,3 @@
+; Mixing unorder atomics and normal loads/stores is
+; current implemented
+define i32 @test6(i32* nocapture noalias %x, i32* nocapture %y) nounwind uwtable ssp {
----------------
Could you also add a test which has:

```
loop:
  store i32 5, i32* %x
  %vala = load atomic i32, i32* %y monotonic, align 4
  store atomic i32 %vala, i32* %z unordered, align 4
  %exitcond = icmp ne i32 %vala, 0
  br i1 %exitcond, label %end, label %loop
```
Note the unordered store is now to `%z` which should also be `noalias`.

================
Comment at: test/Transforms/LICM/atomics.ll:118
@@ +117,3 @@
+; CHECK: load atomic i32, i32* %y
+; CHECK-NEXT: store atomic i32
+}
----------------
Also check that the unordered atomic is still there.


http://reviews.llvm.org/D15592