[PATCH] D83340: Prohibit use of _ExtInt in atomic intrinsic

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D83340#2149100 , @jtmott-intel 
wrote:

> @jfb Ah, I feel I better understand your original question now. Thinking 
> through how we would want `__atomic`s 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83340/new/

https://reviews.llvm.org/D83340



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83340: Prohibit use of _ExtInt in atomic intrinsic

[Ronan Keryell via Phabricator via cfe-commits]

keryell added a comment.

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++?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83340/new/

https://reviews.llvm.org/D83340



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D83340: Prohibit use of _ExtInt in atomic intrinsic

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGd083adb068e7: Prohibit use of _ExtInt in atomic intrinsic 
(authored by jtmott-intel, committed by erichkeane).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83340/new/

https://reviews.llvm.org/D83340

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Sema/builtins.c


Index: clang/test/Sema/builtins.c
===
--- clang/test/Sema/builtins.c
+++ clang/test/Sema/builtins.c
@@ -281,6 +281,42 @@
   __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}}
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -5349,6 +5349,15 @@
   // 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();
+  if (ExtIntValType && !llvm::isPowerOf2_64(ExtIntValType->getNumBits())) {
+Diag(FirstArg->getExprLoc(), diag::err_atomic_builtin_ext_int_size);
+return ExprError();
+  }
+
   return TheCallResult;
 }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -7939,6 +7939,8 @@
 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)">;


Index: clang/test/Sema/builtins.c
===
--- clang/test/Sema/builtins.c
+++ clang/test/Sema/builtins.c
@@ -281,6 +281,42 @@
   __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