Prohibit use of _ExtInt in atomic intrinsic

[jtmott-intel]

Summary

The _ExtInt type allows custom width integers, but the atomic memory access's operand must have a power-of-two size. _ExtInts with non-power-of-two size should not be allowed for atomic intrinsic.

Before this change:

$ cat test.c

typedef unsigned _ExtInt(42) dtype;
void verify_binary_op_nand(dtype* pval1, dtype val2)
{    __sync_nand_and_fetch(pval1, val2); }

$ clang test.c

clang-11: /home/ubuntu/llvm_workspace/llvm/clang/lib/CodeGen/CGBuiltin.cpp:117: llvm::Value* 
EmitToInt(clang::CodeGen::CodeGenFunction&, llvm::Value*, clang::QualType, llvm::IntegerType*): Assertion `V->getType() == IntType' failed.
PLEASE submit a bug report to https://bugs.llvm.org/ and include the crash backtrace, preprocessed source, and associated run script.

After this change:

$ clang test.c

test.c:3:30: error: Atomic memory operand must have a power-of-two size
{    __sync_nand_and_fetch(pval1, val2); }
                       ^

List of the atomic intrinsics that have this problem:

• __sync_fetch_and_add
• __sync_fetch_and_sub
• __sync_fetch_and_or
• __sync_fetch_and_and
• __sync_fetch_and_xor
• __sync_fetch_and_nand
• __sync_nand_and_fetch
• __sync_and_and_fetch
• __sync_add_and_fetch
• __sync_sub_and_fetch
• __sync_or_and_fetch
• __sync_xor_and_fetch
• __sync_fetch_and_min
• __sync_fetch_and_max
• __sync_fetch_and_umin
• __sync_fetch_and_umax
• __sync_val_compare_and_swap
• __sync_bool_compare_and_swap

[jfb]

Are the _Extint uses here as you'd expect too? (using _Atomic): https://reviews.llvm.org/D79279
We have other atomic primitives, such as _atomic_, do these need updates too?

[erichkeane]

clang/lib/Sema/SemaChecking.cpp

5355

const auto *.

5357

use llvm::isPowerOf2_64

[jtmott-intel]

@jfb The __atomic builtins seem to round up to the nearest power of 2, and they appear to work because of that. Is there a specific use case or reproducer you were concerned about?

[jtmott-intel]

clang/lib/Sema/SemaChecking.cpp

5355

The pointer seems to be included in the deduced auto type. Unless there's a style convention that we'd rather write the * ?

[erichkeane]

clang/lib/Sema/SemaChecking.cpp

5355

Yes, the llvm coding guidelines for 'auto' require you use the pointer when its a pointer.

[jtmott-intel]

Addressed review comments.

[erichkeane]

What about powers-of-2 larger than, say, 256 bit? So a 512 bit value. Does this give reasonable codegen?

[jtmott-intel]

@erichkeane Assuming we're back to talking about the __sync functions, I get the following diagnostic.

test.c:22:6: error: address argument to atomic builtin must be a pointer to 1,2,4,8 or 16 byte type ('dtype *' (aka 'unsigned _ExtInt(512) *') invalid)
{    __sync_nand_and_fetch(pval1, val2); }
     ^                     ~~~~~
1 error generated.

Is this what you're referring to? (In highsight I probably could have piggybacked on this diagnostic for the _ExtInt check.)

[erichkeane]

In D83340#2148615, @jtmott-intel wrote:

@erichkeane Assuming we're back to talking about the __sync functions, I get the following diagnostic.

test.c:22:6: error: address argument to atomic builtin must be a pointer to 1,2,4,8 or 16 byte type ('dtype *' (aka 'unsigned _ExtInt(512) *') invalid)
{    __sync_nand_and_fetch(pval1, val2); }
     ^                     ~~~~~
1 error generated.

Is this what you're referring to? (In highsight I probably could have piggybacked on this diagnostic for the _ExtInt check.)

Yep, thats what I was worried about, thanks!

[jtmott-intel]

I don't have commit access. For whoever performs the commit, here's a (draft) commit message:

[clang] Prohibit use of non-power-of-2 _ExtInt with __sync builtins

[jfb]

In D83340#2148089, @jtmott-intel wrote:

@jfb The __atomic builtins seem to round up to the nearest power of 2, and they appear to work because of that. Is there a specific use case or reproducer you were concerned about?

This is ABI, so I don’t want to ship “whatever happens to be implemented”. It ought to be well thought out. Specifically for atomic, GCC and LLVM diverge on those details, and I wouldn’t want this new feature to continue the divergence.

[jtmott-intel]

@jfb Ah, I feel I better understand your original question now. Thinking through how we would want __atomics to behave is great, and I'm genuinely glad someone's championing it. It also sounds a *smidgen* beyond the scope of what I was trying to accomplish with this particular patch. :-) This patch doesn't alter the behavior of the __atomic builtins one way or the other. It alters the behavior of the __sync builtins only to change a debug assert failure into a diagnostic error. I believe the _ExtInt authors are tracking new bugs related to that feature. A new bug report could keep the discussion going and keep the issue visible.

[erichkeane]

In D83340#2149100, @jtmott-intel wrote:

@jfb Ah, I feel I better understand your original question now. Thinking through how we would want __atomics to behave is great, and I'm genuinely glad someone's championing it. It also sounds a *smidgen* beyond the scope of what I was trying to accomplish with this particular patch. :-) This patch doesn't alter the behavior of the __atomic builtins one way or the other. It alters the behavior of the __sync builtins only to change a debug assert failure into a diagnostic error. I believe the _ExtInt authors are tracking new bugs related to that feature. A new bug report could keep the discussion going and keep the issue visible.

Jeff: Please submit a patch to at least disable ExtInt for these atomic builtins, that way we can design their behavior later. I agree that this shouldn't block this patch, but we need ASAP.

[keryell]

The idea of this _ExtInt is to have some extensions.
Since it is an extension, why preventing its use?
For example if I want my 18 bit FPGA BRAM to be accessed atomically?
Or is there an assumption that atomic access can be enabled back with some other mode, such as SYCL or HLS C++?

[erichkeane]

In D83340#2149100, @jtmott-intel wrote:

@jfb Ah, I feel I better understand your original question now. Thinking through how we would want __atomics to behave is great, and I'm genuinely glad someone's championing it. It also sounds a *smidgen* beyond the scope of what I was trying to accomplish with this particular patch. :-) This patch doesn't alter the behavior of the __atomic builtins one way or the other. It alters the behavior of the __sync builtins only to change a debug assert failure into a diagnostic error. I believe the _ExtInt authors are tracking new bugs related to that feature. A new bug report could keep the discussion going and keep the issue visible.

Jeff: Please submit a patch to at least disable ExtInt for these atomic builtins, that way we can design their behavior later. I agree that this shouldn't block this patch, but we need it soon.

In D83340#2151198, @keryell wrote:

The idea of this _ExtInt is to have some extensions.
Since it is an extension, why preventing its use?
For example if I want my 18 bit FPGA BRAM to be accessed atomically?
Or is there an assumption that atomic access can be enabled back with some other mode, such as SYCL or HLS C++?

The idea is that until we do an audit/design for the way atomic builtins are supposed to work, we should leave them restricted as they can bind us to a sub-optimal implementation via ABI. If you have a platform/sub language that spends a good amount of time designing/defining the mechanism in an informed way(rather than organically inheriting them), enabling it for that situation is a perfectly reasonable thing to do.

In the meantime, we would like to disable things that aren't perfectly sensible in order to preserve the ability to do the above.

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_atomic_builtin_must_be_pointer_intfltptr : Error<
   "address argument to atomic builtin must be a pointer to integer,"
   " floating-point or pointer (%0 invalid)">;
 def err_atomic_builtin_pointer_size : Error<
   "address argument to atomic builtin must be a pointer to 1,2,4,8 or 16 byte "
   "type (%0 invalid)">;
 def err_atomic_exclusive_builtin_pointer_size : Error<
   "address argument to load or store exclusive builtin must be a pointer to"
   " 1,2,4 or 8 byte type (%0 invalid)">;
+def err_atomic_builtin_ext_int_size : Error<
+  "Atomic memory operand must have a power-of-two size">;
 def err_atomic_op_needs_atomic : Error<
   "address argument to atomic operation must be a pointer to _Atomic "
   "type (%0 invalid)">;
 def err_atomic_op_needs_non_const_atomic : Error<
   "address argument to atomic operation must be a pointer to non-%select{const|constant}0 _Atomic "
   "type (%1 invalid)">;
 def err_atomic_op_needs_non_const_pointer : Error<
   "address argument to atomic operation must be a pointer to non-const "

clang/lib/Sema/SemaChecking.cpp

ExprResult PromotedCall = ImpCastExprToType(NewDRE, CalleePtrTy,
                                               CK_BuiltinFnToFnPtr);
   TheCall->setCallee(PromotedCall.get());
 
   // Change the result type of the call to match the original value type. This
   // is arbitrary, but the codegen for these builtins ins design to handle it
   // gracefully.
   TheCall->setType(ResultType);
 
+  // Prohibit use of _ExtInt with atomic builtins.
+  // The arguments would have already been converted to the first argument's
+  // type, so only need to check the first argument.
+  const auto *ExtIntValType = ValType->getAs<ExtIntType>();
+  if (ExtIntValType && !llvm::isPowerOf2_64(ExtIntValType->getNumBits())) {
+    Diag(FirstArg->getExprLoc(), diag::err_atomic_builtin_ext_int_size);
+    return ExprError();
+  }
+
   return TheCallResult;
 }
 
 /// SemaBuiltinNontemporalOverloaded - We have a call to
 /// __builtin_nontemporal_store or __builtin_nontemporal_load, which is an
 /// overloaded function based on the pointer type of its last argument.
 ///
 /// This function goes through and does final semantic checking for these

clang/test/Sema/builtins.c

void test_builtin_launder(char *p, void *vp, const void *cvp,
   __builtin_launder(fn); // expected-error {{function pointer argument to '__builtin_launder' is not allowed}}
 }
 
 void test21(const int *ptr) {
   __sync_fetch_and_add(ptr, 1); // expected-error{{address argument to atomic builtin cannot be const-qualified ('const int *' invalid)}}
   __atomic_fetch_add(ptr, 1, 0);  // expected-error {{address argument to atomic operation must be a pointer to non-const type ('const int *' invalid)}}
 }
 
+void test_ei_i42i(_ExtInt(42) *ptr, int value) {
+  __sync_fetch_and_add(ptr, value); // expected-error {{Atomic memory operand must have a power-of-two size}}
+  // expected-warning@+1 {{the semantics of this intrinsic changed with GCC version 4.4 - the newer semantics are provided here}}
+  __sync_nand_and_fetch(ptr, value); // expected-error {{Atomic memory operand must have a power-of-two size}}
+}
+
+void test_ei_i64i(_ExtInt(64) *ptr, int value) {
+  __sync_fetch_and_add(ptr, value); // expect success
+  // expected-warning@+1 {{the semantics of this intrinsic changed with GCC version 4.4 - the newer semantics are provided here}}
+  __sync_nand_and_fetch(ptr, value); // expect success
+}
+
+void test_ei_ii42(int *ptr, _ExtInt(42) value) {
+  __sync_fetch_and_add(ptr, value); // expect success
+  // expected-warning@+1 {{the semantics of this intrinsic changed with GCC version 4.4 - the newer semantics are provided here}}
+  __sync_nand_and_fetch(ptr, value); // expect success
+}
+
+void test_ei_ii64(int *ptr, _ExtInt(64) value) {
+  __sync_fetch_and_add(ptr, value); // expect success
+  // expected-warning@+1 {{the semantics of this intrinsic changed with GCC version 4.4 - the newer semantics are provided here}}
+  __sync_nand_and_fetch(ptr, value); // expect success
+}
+
+void test_ei_i42i42(_ExtInt(42) *ptr, _ExtInt(42) value) {
+  __sync_fetch_and_add(ptr, value); // expected-error {{Atomic memory operand must have a power-of-two size}}
+  // expected-warning@+1 {{the semantics of this intrinsic changed with GCC version 4.4 - the newer semantics are provided here}}
+  __sync_nand_and_fetch(ptr, value); // expected-error {{Atomic memory operand must have a power-of-two size}}
+}
+
+void test_ei_i64i64(_ExtInt(64) *ptr, _ExtInt(64) value) {
+  __sync_fetch_and_add(ptr, value); // expect success
+  // expected-warning@+1 {{the semantics of this intrinsic changed with GCC version 4.4 - the newer semantics are provided here}}
+  __sync_nand_and_fetch(ptr, value); // expect success
+}
+
 void test22(void) {
   (void)__builtin_signbit(); // expected-error{{too few arguments to function call, expected 1, have 0}}
   (void)__builtin_signbit(1.0, 2.0, 3.0); // expected-error{{too many arguments to function call, expected 1, have 3}}
   (void)__builtin_signbit(1); // expected-error {{floating point classification requires argument of floating point type (passed in 'int')}}
   (void)__builtin_signbit(1.0);
   (void)__builtin_signbit(1.0f);
   (void)__builtin_signbit(1.0L);