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

[Blower, Melanie via cfe-commits]

Yes the IR looks a bit odd. I should use a simpler case with a global _Atomic int a instead of the parameter. The store is the parameter value a, if I move that to a global then it won't be as confusing. 

> -----Original Message-----
> From: JF Bastien via Phabricator [mailto:reviews at reviews.llvm.org]
> Sent: Wednesday, March 13, 2019 12:57 PM
> To: Blower, Melanie <melanie.blower at intel.com>; cfe-commits at lists.llvm.org;
> Keane, Erich <erich.keane at intel.com>; richard at metafoo.co.uk;
> rnk at google.com; jfbastien at apple.com
> Cc: jdoerfert at anl.gov; mlekena at skidmore.edu; blitzrakete at gmail.com;
> shenhan at google.com
> Subject: [PATCH] D59307: Patch llvm bug 41033 concerning atomicity of
> statement expressions
> 
> jfb requested changes to this revision.
> jfb added a comment.
> This revision now requires changes to proceed.
> 
> I think you also want to test C++ `std::atomic` as well as regular `volatile`.
> 
> 
> 
> ================
> 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
> ----------------
> Why is there a store to `a` here?
[Blower, Melanie] Yes the IR looks a bit odd. I can use a simpler case with a global _Atomic int a instead of the parameter. But 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. Without the StmtExpr, there's just the atomic load, which is then stored into b.

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
}
> 
> 
> ================
> 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 }
> ----------------
> What's in `%tmp`? I'd expect the value loaded from `a` to be stored to `b`.
> 
> 
> Repository:
>   rC Clang
> 
> CHANGES SINCE LAST ACTION
>   https://reviews.llvm.org/D59307/new/
> 
> https://reviews.llvm.org/D59307
> 
>