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
