[PATCH] D19440: [GVN] Do local FRE for unordered atomic loads

[JF Bastien via llvm-commits]

jfb added inline comments.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:1231
@@ -1229,3 +1230,3 @@
     if (StoreInst *DepSI = dyn_cast<StoreInst>(DepInfo.getInst())) {
-      if (Address) {
+      if (Address && LI->isAtomic() <= DepSI->isAtomic()) {
         int Offset =
----------------
Why `<=` here and below? I'd like a comment explaining the rationale.

================
Comment at: lib/Transforms/Scalar/GVN.cpp:1774
@@ -1759,2 +1773,3 @@
 
-  if (!L->isSimple())
+  // This code hasn't been auditted for ordered or volatile memory access
+  if (!L->isUnordered())
----------------
mgrang wrote:
> Comment is missing a period (.) at the end.
Typo "audited".

================
Comment at: test/Transforms/GVN/atomic.ll:111
@@ +110,3 @@
+
+; CHECK-LABEL: @test12(
+define i32 @test12(i1 %B, i32* %P1, i32* %P2) {
----------------
What is this one testing? That unrelated loads stay there?

================
Comment at: test/Transforms/GVN/atomic.ll:123
@@ +122,3 @@
+; CHECK-LABEL: @test13(
+; atomic to non-atomic forwarding is legal
+define i32 @test13(i1 %B, i32* %P1) {
----------------
So I understand: this isn't a new behavior, right? It's a good test, but your change doesn't make this test pass, it already did before. test13b is the new one :)

================
Comment at: test/Transforms/GVN/atomic.ll:124
@@ +123,3 @@
+; atomic to non-atomic forwarding is legal
+define i32 @test13(i1 %B, i32* %P1) {
+  %a = load atomic i32, i32* %P1 seq_cst, align 4
----------------
`%B` is unused here and in tests below.

================
Comment at: test/Transforms/GVN/atomic.ll:133
@@ +132,3 @@
+
+; CHECK-LABEL: @test13b
+define i32 @test13b(i32* %P1) {
----------------
`CHECK-LABEL` where and below should all have the starting paren (e.g. `@test13b(`) because some of the tests otherwise have the same prefix and would match the wrong label.

================
Comment at: test/Transforms/GVN/atomic.ll:208
@@ +207,3 @@
+  ret void
+}
+
----------------
Also test with `fence singlethread`.

================
Comment at: test/Transforms/GVN/atomic.ll:288
@@ +287,3 @@
+
+; We're currently conservative about widdening
+define i64 @widden1(i32* %P1) {
----------------
Typo "widening".


http://reviews.llvm.org/D19440