[PATCH] D59307: Patch llvm bug 41033 concerning atomicity of statement expressions

[Melanie Blower via Phabricator via cfe-commits]

mibintc marked 2 inline comments as done.
mibintc added a comment.

Thanks for test suggestions, will follow up



================
Comment at: test/Sema/atomic-expr-stmt.c:7
+  //CHECK: %atomic-load = load atomic i32, i32* %a.addr seq_cst, align 4
+  //CHECK: store atomic i32 %atomic-load, i32* %tmp seq_cst, align 4
+  //CHECK: %0 = load i32, i32* %tmp, align 4
----------------
jfb wrote:
> Why is there a store to `a` here?
Yes the IR is odd, I will rewrite with a as global _Atomic int, not a parameter


================
Comment at: test/Sema/atomic-expr-stmt.c:9
+  //CHECK: %0 = load i32, i32* %tmp, align 4
+  //CHECK: store i32 %0, i32* %b.addr, align 4
+}
----------------
jfb wrote:
> What's in `%tmp`? I'd expect the value loaded from `a` to be stored to `b`.
It does seem like the "store atomic" to tmp shouldn't exist.  The %tmp is the value returned by the StmtExpr. Since the atomic load has already occurred, the value returned by StmtExpr could just be %atomic-load. If I compare this IR to a test case without the StmtExpr, there's just the atomic load of a which is then stored into b.  I think the value of the atomic-load is still hanging on to the atomic attribute but it should be dropped. 

I changed the test like this, and the IR follows _Atomic int a; void test_assign(int b) {
  // assignment is OK
  b = ({a;});
}

; Function Attrs: noinline nounwind optnone define void @test_assign(i32 %b) #0 {
entry:
  %b.addr = alloca i32, align 4
  %tmp = alloca i32, align 4
  store i32 %b, i32* %b.addr, align 4
  %atomic-load = load atomic i32, i32* @a seq_cst, align 4
  store atomic i32 %atomic-load, i32* %tmp seq_cst, align 4
  %0 = load i32, i32* %tmp, align 4
  store i32 %0, i32* %b.addr, align 4
  ret void
}



Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59307/new/

https://reviews.llvm.org/D59307