[PATCH] D15352: [EarlyCSE] DSE of atomic unordered stores

[JF Bastien via llvm-commits]

jfb added a comment.

I'm not sure this is good to go as-is.


================
Comment at: lib/Transforms/Scalar/EarlyCSE.cpp:690
@@ +689,3 @@
+        // location with no intervening loads.  Delete the earlier store.
+        // At the moment, we don't removal ordered stores, but do remove
+        // unordered atomic stores.  There's no special requirement about
----------------
Typo "removal".

================
Comment at: test/Transforms/EarlyCSE/atomics.ll:131
@@ +130,3 @@
+  store atomic i32 3, i32* %P1 unordered, align 4
+  store i32 0, i32* %P1, align 4
+  ret void
----------------
I think the store you keep still needs to be atomic, i.e. you're "sinking" the atomicity to all post-dominating stores. Let's ignore that this is `unordered` (silly example) and imagine it's a `store release`: in that case the writer could have been writing a non-zero value, then racing with itself and writing another non-zero value to the same location. The reader observes the transition to non-zero (yes it's racy *which* non-zero is seen, but one is seen). If you drop `release` then the algorithm becomes incorrect.

```
void writer(Payload p, atomic<int> *flag) {
  p->update();
  flag->write(1, release);
  // ...
  *reinterpret_cast<int*>(flag) = 2; // WTF?
}
void reader(Payload p, atomic<int> *flag) {
  while (!flag->load(acquire)) ;
  p->process();
}
```

In the above example `p->process()` isn't guaranteed to see the latest state from `p`. Yes the code is already UB, but we don't need to be silly.

Seen another way, replace `reinterpret_cast` with a `flag->store(2, relaxed)` and then we're well-defined, you can eliminate the `store release`, but you have to "sink" the `release` to the later store.

================
Comment at: test/Transforms/EarlyCSE/atomics.ll:159
@@ +158,3 @@
+; CHECK-LABEL: @test26
+; CHECK-NEXT: store atomic
+; CHECK-NEXT: ret
----------------
Can you check that `3` is the value stored here? We should probably try to avoid unexpected things even when we're allowed to do the unexpected (and document that we're doing this).

================
Comment at: test/Transforms/EarlyCSE/atomics.ll:170
@@ +169,3 @@
+; CHECK-NEXT: store atomic
+; CHECK-DAG: release
+; CHECK-NEXT: ret
----------------
Why not just:
```
; CHECK-LABEL: @test27
; CHECK-NEXT: store atomic i32 3, i32* %p1 release
; CHECK-NEXT: ret
```
?

================
Comment at: test/Transforms/EarlyCSE/atomics.ll:177
@@ +176,3 @@
+
+; Can DSE a normal store in favor of a ordered one
+define void @test28(i1 %B, i32* %P1, i32* %P2) {
----------------
This test isn't testing what the documentation says it is.

================
Comment at: test/Transforms/EarlyCSE/atomics.ll:182
@@ +181,3 @@
+; CHECK-DAG: release
+; CHECK-NEXT: ret
+  store atomic i32 0, i32* %P1 unordered, align 4
----------------
Ditto on `CHECK`.


http://reviews.llvm.org/D15352