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

[Blower, Melanie via cfe-commits]

Sorry I formatted my reply badly, there are responses inline in the previous message

> -----Original Message-----
> From: Blower, Melanie
> Sent: Wednesday, March 13, 2019 2:06 PM
> To: 'reviews+D59307+public+153a08d52e63c375 at reviews.llvm.org'
> <reviews+D59307+public+153a08d52e63c375 at reviews.llvm.org>; 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: RE: [PATCH] D59307: Patch llvm bug 41033 concerning atomicity of
> statement expressions
> 
> 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
> >
> >