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+153a08d52e63c...@reviews.llvm.org' > <reviews+d59307+public+153a08d52e63c...@reviews.llvm.org>; cfe- > comm...@lists.llvm.org; Keane, Erich <erich.ke...@intel.com>; > rich...@metafoo.co.uk; r...@google.com; jfbast...@apple.com > Cc: jdoerf...@anl.gov; mlek...@skidmore.edu; blitzrak...@gmail.com; > shen...@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:revi...@reviews.llvm.org] > > Sent: Wednesday, March 13, 2019 12:57 PM > > To: Blower, Melanie <melanie.blo...@intel.com>; > > cfe-commits@lists.llvm.org; Keane, Erich <erich.ke...@intel.com>; > > rich...@metafoo.co.uk; r...@google.com; jfbast...@apple.com > > Cc: jdoerf...@anl.gov; mlek...@skidmore.edu; blitzrak...@gmail.com; > > shen...@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 > > > > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
