Patch llvm bug 41033 concerning atomicity of statement expressions

[mibintc]

Summary

Clang considers the type of a statement expression that returns the value of an _Atomic(ty) to be atomic, and this makes the statement expression incompatible with other expressions of type ty, for example in assignment or comparison. This patch drops the atomic attribute before creating the StmtExpr.

I noticed that unlike gcc, clang doesn't allow StmtExpr on the left hand side of an assignment, I agree with clang.

I checked with Clark Nelson about the legitimacy of dropping the atomicity, he said "Atomicity has to do with the way data is transferred between a processor and its memory. Once it has been loaded, it's just data."

[jfb]

I think you also want to test C++ std::atomic as well as regular volatile.

test/Sema/atomic-expr-stmt.c

7

Why is there a store to a here?

9

What's in %tmp? I'd expect the value loaded from a to be stored to b.

[jfb]

Thinking some more, this is really a silly gotcha in code. We should probably follow what we're about to do with wg21.link/P1152 and ban chaining. Statement expressions should therefore not be allowed to return _Atomic, std::atomic, nor volatile.

[jfb]

From an offline discussion, I'm told that @jwakely uses this in GCC headers and expects some semantics from it? Jonathan, why?!?!

[mibintc]

Thanks for test suggestions, will follow up

test/Sema/atomic-expr-stmt.c

7

Yes the IR is odd, I will rewrite with a as global _Atomic int, not a parameter

9

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

}

[mibintc]

In D59307#1427663, @jfb wrote:

Thinking some more, this is really a silly gotcha in code. We should probably follow what we're about to do with wg21.link/P1152 and ban chaining. Statement expressions should therefore not be allowed to return _Atomic, std::atomic, nor volatile.

If the atomic load which is the value of the StmtExpr drops the atomic attribute then this would trivially be true, and there would be no need to add a restriction. According to what Clark says, once the value is loaded it's just data.

[mibintc]

In D59307#1427665, @jfb wrote:

From an offline discussion, I'm told that @jwakely uses this in GCC headers and expects some semantics from it? Jonathan, why?!?!

Yes this is how I stumbled into the issue.

[erichkeane]

This is my concern here:
https://godbolt.org/z/icS4fa
The patch will change template instantiation.

GCC doesn't seem to allow using _Atomic in C++ mode, which is perhaps a necessary part of this solution? I see we already consider overload sets with int and _Atomic(int) to be ambiguous (in addition to not considering conversion operators of _Atomic(int) at all??), so it is limited to type deduction. I could see this causing problems, since specializations of templates are allowed on int and _Atomic(int) despite not being used in overload sets; https://godbolt.org/z/oMAvpz

[mibintc]

In D59307#1427644, @jfb wrote:

I think you also want to test C++ std::atomic ...

The bug described in https://bugs.llvm.org/show_bug.cgi?id=41033 doesn't occur using C++ atomic, I rewrote the test case this way, and it compiles without error. The "load" operation returns int since all the atomic operations occur under the covers using builtins. Does this convey the test you have in mind?

#include <atomic>
int fum(int y) {

std::atomic<int> x(1);
y = ({x.load();});

}

[jfb]

In D59307#1428101, @mibintc wrote:

In D59307#1427644, @jfb wrote:

I think you also want to test C++ std::atomic ...

The bug described in https://bugs.llvm.org/show_bug.cgi?id=41033 doesn't occur using C++ atomic, I rewrote the test case this way, and it compiles without error. The "load" operation returns int since all the atomic operations occur under the covers using builtins. Does this convey the test you have in mind?

#include <atomic>
int fum(int y) {

std::atomic<int> x(1);
y = ({x.load();});

}

What I had in mind with atomic isn't relevant, because it would try to call atomic(const atomic&) = delete;. Your test case here isn't something I'm worried about.

volatile is still and issue.

I'm still not sure this is something we want.

[mibintc]

In D59307#1428145, @jfb wrote:

In D59307#1428101, @mibintc wrote:

In D59307#1427644, @jfb wrote:

I think you also want to test C++ std::atomic ...

The bug described in https://bugs.llvm.org/show_bug.cgi?id=41033 doesn't occur using C++ atomic, I rewrote the test case this way, and it compiles without error. The "load" operation returns int since all the atomic operations occur under the covers using builtins. Does this convey the test you have in mind?

#include <atomic>
int fum(int y) {

std::atomic<int> x(1);
y = ({x.load();});

}

What I had in mind with atomic isn't relevant, because it would try to call atomic(const atomic&) = delete;. Your test case here isn't something I'm worried about.

volatile is still and issue.

I'm still not sure this is something we want.

Now I think the problem needs to be fixed at the atomic load, not in this place. Thanks for pointing out the odd use of tmp. I'm going to abandon this patch.

[mibintc]

In D59307#1427853, @erichkeane wrote:

This is my concern here:
https://godbolt.org/z/icS4fa
The patch will change template instantiation.

GCC doesn't seem to allow using _Atomic in C++ mode, which is perhaps a necessary part of this solution? I see we already consider overload sets with int and _Atomic(int) to be ambiguous (in addition to not considering conversion operators of _Atomic(int) at all??), so it is limited to type deduction. I could see this causing problems, since specializations of templates are allowed on int and _Atomic(int) despite not being used in overload sets; https://godbolt.org/z/oMAvpz

Recognizing _Atomic in c++ mode is an extension right? Probably it needs more thought, for example the test case you provided. Thank you

[mibintc]

The bug should be fixed at the atomic-load (drop _Atomic qualifier) then this patch won't be needed in StmtExpr

lib/Sema/SemaExpr.cpp

Sema::ActOnStmtExpr(SourceLocation LPLoc, Stmt *SubStmt,
   // as the type of the stmtexpr.
   QualType Ty = Context.VoidTy;
   bool StmtExprMayBindToTemp = false;
   if (!Compound->body_empty()) {
     if (const auto *LastStmt = dyn_cast<ValueStmt>(Compound->body_back())) {
       if (const Expr *Value = LastStmt->getExprStmt()) {
         StmtExprMayBindToTemp = true;
         Ty = Value->getType();
+        if (const AtomicType *AtomicRHS = Ty->getAs<AtomicType>())
+          Ty = AtomicRHS->getValueType();
       }
     }
   }
 
   // FIXME: Check that expression type is complete/non-abstract; statement
   // expressions are not lvalues.
   Expr *ResStmtExpr = new (Context) StmtExpr(Compound, Ty, LPLoc, RPLoc);
   if (StmtExprMayBindToTemp)

test/Sema/atomic-expr-stmt.c

+// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm < %s | FileCheck %s
+void test_assign(_Atomic int a, int b) {
+  //CHECK: define void @test_assign(i32 %a, i32 %b)
+  // assignment is OK
+  b = ({a;});
+  //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
+  //CHECK: store i32 %0, i32* %b.addr, align 4
+}
+
+int test_compare(_Atomic int x, int y) {
+  //CHECK: define i32 @test_compare(i32 %x, i32 %y)
+  // comparison is OK
+  //CHECK: %0 = load i32, i32* %y.addr, align 4
+  //CHECK: %atomic-load = load atomic i32, i32* %x.addr seq_cst, align 4
+  //CHECK: store atomic i32 %atomic-load, i32* %tmp seq_cst, align 4
+  //CHECK: %1 = load i32, i32* %tmp, align 4
+  //CHECK: %cmp = icmp slt i32 %0, %1
+  if (y <({x;}))
+    return 0;
+  else return 1;
+}