[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG61d065e21ff3: Let clang atomic builtins fetch add/sub 
support floating point types (authored by yaxunl).
Herald added a project: clang.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/fp-atomic-ops.c
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *f, atomic_double *d, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -57,37 +60,38 @@
 
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(f, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+  (int)__opencl_atomic_store(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
 
   int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
   int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(f, 1.0f, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_fetch_add(d, 1.0, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_and(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
 
   __opencl_atomic_fetch_min(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_max(i, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_min(d, 1, memory_order_seq

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 335489.
yaxunl added a comment.

revised by John's comments


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/fp-atomic-ops.c
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *f, atomic_double *d, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -57,37 +60,38 @@
 
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(f, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+  (int)__opencl_atomic_store(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
 
   int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
   int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(f, 1.0f, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_fetch_add(d, 1.0, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_and(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
 
   __opencl_atomic_fetch_min(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_max(i, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_min(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
-  __opencl_atomic_fetch_max

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Alright, mostly looks good.




Comment at: clang/lib/Sema/SemaChecking.cpp:5011
+!ValType->isFloatingType()) {
+  Diag(ExprRange.getBegin(), 
diag::err_atomic_op_needs_atomic_int_ptr_or_fp)
   << IsC11 << Ptr->getType() << Ptr->getSourceRange();

yaxunl wrote:
> rjmccall wrote:
> > yaxunl wrote:
> > > rjmccall wrote:
> > > > Does LLVM support atomics on all floating-point types?
> > > LLVM IR parser requires atomicrmw value operand must have size of power 
> > > of 2, therefore LLVM does not support atomicrmw on x86_fp80 which has 
> > > size of 80 bytes. LLVM supports atomicrmw on all other floating-point 
> > > types (bfloat, half, float, double, fp128, ppc_fp128).
> > Okay.  So this needs to check the underlying FP semantics and disallow 
> > atomics on unsupported types.
> will do
Could you extract this whole condition into a function and make it a bit more 
readable?


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 335366.
yaxunl marked an inline comment as done.
yaxunl added a comment.

Revised by John's comments. Do not allow atomic fetch add with x86_fp80.


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/fp-atomic-ops.c
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *f, atomic_double *d, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -57,37 +60,38 @@
 
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(f, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+  (int)__opencl_atomic_store(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
 
   int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
   int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(f, 1.0f, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_fetch_add(d, 1.0, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_and(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
 
   __opencl_atomic_fetch_min(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_max(i, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_min(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic ato

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 2 inline comments as done.
yaxunl added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:5011
+!ValType->isFloatingType()) {
+  Diag(ExprRange.getBegin(), 
diag::err_atomic_op_needs_atomic_int_ptr_or_fp)
   << IsC11 << Ptr->getType() << Ptr->getSourceRange();

rjmccall wrote:
> yaxunl wrote:
> > rjmccall wrote:
> > > Does LLVM support atomics on all floating-point types?
> > LLVM IR parser requires atomicrmw value operand must have size of power of 
> > 2, therefore LLVM does not support atomicrmw on x86_fp80 which has size of 
> > 80 bytes. LLVM supports atomicrmw on all other floating-point types 
> > (bfloat, half, float, double, fp128, ppc_fp128).
> Okay.  So this needs to check the underlying FP semantics and disallow 
> atomics on unsupported types.
will do


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:5011
+!ValType->isFloatingType()) {
+  Diag(ExprRange.getBegin(), 
diag::err_atomic_op_needs_atomic_int_ptr_or_fp)
   << IsC11 << Ptr->getType() << Ptr->getSourceRange();

yaxunl wrote:
> rjmccall wrote:
> > Does LLVM support atomics on all floating-point types?
> LLVM IR parser requires atomicrmw value operand must have size of power of 2, 
> therefore LLVM does not support atomicrmw on x86_fp80 which has size of 80 
> bytes. LLVM supports atomicrmw on all other floating-point types (bfloat, 
> half, float, double, fp128, ppc_fp128).
Okay.  So this needs to check the underlying FP semantics and disallow atomics 
on unsupported types.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:5011
+!ValType->isFloatingType()) {
+  Diag(ExprRange.getBegin(), 
diag::err_atomic_op_needs_atomic_int_ptr_or_fp)
   << IsC11 << Ptr->getType() << Ptr->getSourceRange();

rjmccall wrote:
> Does LLVM support atomics on all floating-point types?
LLVM IR parser requires atomicrmw value operand must have size of power of 2, 
therefore LLVM does not support atomicrmw on x86_fp80 which has size of 80 
bytes. LLVM supports atomicrmw on all other floating-point types (bfloat, half, 
float, double, fp128, ppc_fp128).


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/lib/Sema/SemaChecking.cpp:5011
+!ValType->isFloatingType()) {
+  Diag(ExprRange.getBegin(), 
diag::err_atomic_op_needs_atomic_int_ptr_or_fp)
   << IsC11 << Ptr->getType() << Ptr->getSourceRange();

Does LLVM support atomics on all floating-point types?


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D71726#2645269 , @tra wrote:

> @jyknight - James, do you have further concerns about the patch?

I separated the change about diagnosing unaligned atomics for amdgpu to 
https://reviews.llvm.org/D99201 since these two changes are orthogonal.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 332730.
yaxunl edited the summary of this revision.
yaxunl added a comment.

separate diagnosing unaligned atomc for amdgpu to another review.


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/fp-atomic-ops.c
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *f, atomic_double *d, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -57,37 +60,38 @@
 
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(f, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+  (int)__opencl_atomic_store(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
 
   int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
   int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(f, 1.0f, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_fetch_add(d, 1.0, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_and(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
 
   __opencl_atomic_fetch_min(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_max(i, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_min(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

@jyknight - James, do you have further concerns about the patch?




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6454
+bool DiagAtomicLibCall = true;
+for (auto *A : Args.filtered(options::OPT_W_Joined)) {
+  if (StringRef(A->getValue()) == "no-error=atomic-alignment")

If we rely on promoting the warnings to errors for correctness, I think we may 
need a more robust mechanism to enforce that than trying to guess the state 
based on provided options.
E.g. can these diagnostics be enabled/disabled with a wider scope option like 
`-W[no-]extra` or `-W[no-]all`?

Maybe we should add a cc1-only option `--enforce-atomic-alignment` and use that 
to determine if misalignment should be an error at the point where we issue the 
diagnostics?




Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6457
+DiagAtomicLibCall = false;
+  if (StringRef(A->getValue()) == "error=atomic-alignment")
+DiagAtomicLibCall = true;

This should be `else if`, or,  maybe use `llvm::StringSwitch()`instead:
```
DiagAtomicLibCall = llvm::StringSwitch(A->getValue())
   .Case("no-error=atomic-alignment", false)
   .Case("error=atomic-alignment", true)
   .Default(DiagAtomicLibCall)
```


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 332658.
yaxunl added a comment.

Re-use existing warning instead of introducing new diagnostics.

Ping. Can some one help review this patch? I believe all comments addressed. 
Thanks.


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/fp-atomic-ops.c
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Driver/hip-options.hip
  clang/test/Sema/atomic-ops.c
  clang/test/SemaCUDA/amdgpu-atomic-ops.cu
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *f, atomic_double *d, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -57,37 +60,38 @@
 
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(f, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+  (int)__opencl_atomic_store(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
 
   int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
   int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(f, 1.0f, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_fetch_add(d, 1.0, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_and(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
 
   __opencl_atomic_fetch_min(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_max(i, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_min(d, 1, 

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

@rjmccall @jyknight Ping. Any further concerns? Thanks.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

@jyknight @rjmccall ping. diagnostic issue addressed.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

ping


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 322005.
yaxunl edited the summary of this revision.
yaxunl added a comment.

Revised by James, Artem, and John's comments.


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticFrontendKinds.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/AMDGPU.cpp
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/fp-atomic-ops.c
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaCUDA/amdgpu-atomic-ops.cu
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *f, atomic_double *d, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -57,37 +60,38 @@
 
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(f, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+  (int)__opencl_atomic_store(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
 
   int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
   int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(f, 1.0f, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_fetch_add(d, 1.0, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_and(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
 
   __opencl_atomic_fetch_min(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_max(i, 1, memory_order_seq_cst, memory_scope_work_group);
-  __o

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 2 inline comments as done.
yaxunl added a comment.

In D71726#2537378 , @jyknight wrote:

> In D71726#2537101 , @yaxunl wrote:
>
>> For amdgpu target, we do need diagnose unsupported atomics (not limited to 
>> fp atomics) since we do not support libcall due to ISA level linking not 
>> supported. This is something we cannot fix in a short time and we would 
>> rather diagnose it than confusing the users with missing symbols in lld.
>
> If this is limited simply to not supporting oversized or misaligned atomics, 
> I'd find that a lot less objectionable. At that point you just need a single 
> boolean variable/accessor for whether the target can support atomic library 
> calls. I note that we already have warning messages: 
> warn_atomic_op_misaligned and warn_atomic_op_oversized. Maybe those can just 
> be promoted to errors on AMDGPU.

Good points. Will do.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D71726#2537101 , @yaxunl wrote:

> For amdgpu target, we do need diagnose unsupported atomics (not limited to fp 
> atomics) since we do not support libcall due to ISA level linking not 
> supported. This is something we cannot fix in a short time and we would 
> rather diagnose it than confusing the users with missing symbols in lld.

If this is limited simply to not supporting oversized or misaligned atomics, 
I'd find that a lot less objectionable. At that point you just need a single 
boolean variable/accessor for whether the target can support atomic library 
calls. I note that we already have warning messages: warn_atomic_op_misaligned 
and warn_atomic_op_oversized. Maybe those can just be promoted to errors on 
AMDGPU.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D71726#2537101 , @yaxunl wrote:

> In D71726#2537054 , @tra wrote:
>
>> In D71726#2536966 , @jyknight wrote:
>>
>>> My concern is that this is treating a backend _bug_ as if it were just an 
>>> optional feature. But it's not the case that it might be reasonable to 
>>> either implement or not implement this in a backend -- it should be 
>>> implemented, and those that don't are buggy.
>>>
>>> I'd be happier with just having an ISEL failure when you try to use fp 
>>> atomics on broken targets, rather than adding all this code and 
>>> configuration to Clang in order to avoid that. (And, of course, the target 
>>> maintainers should also fix them)
>>
>> +1. I agree with James.
>>
>> Removing code is often harder than adding it. When you're adding things, 
>> you're the only user. Once things are in, they will start growing 
>> dependencies that will need to be dealt with if you ever want to remove the 
>> code.
>>
>> Clean solution that works for AMDGPU only for now is better than a 
>> potentially permanent workaround.
>
> For amdgpu target, we do need diagnose unsupported atomics (not limited to fp 
> atomics) since we do not support libcall due to ISA level linking not 
> supported. This is something we cannot fix in a short time and we would 
> rather diagnose it than confusing the users with missing symbols in lld.

Diagnosing that you don't support atomics your target can't reasonably support 
is completely fine.  (You could actually actually inline a locking approach if 
you really wanted to, though; Microsoft's `std::atomic` does that in the 
general case, although admittedly that's library code.)  I would like to 
understand whether that's really type-specific or just size-specific, though, 
and I don't think we've gotten a plain answer about that.  Is it true that 
amdgpu simply does not have a generic cmpxchg?

> For other targets, I can make changes to assume fp atomics are supported if 
> width is within max inline atomic width of the target. Basically this will 
> let fp atomics emitted for these targets and assuming middle end or backend 
> will handle them properly.

I think that's reasonable.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D71726#2537054 , @tra wrote:

> In D71726#2536966 , @jyknight wrote:
>
>> My concern is that this is treating a backend _bug_ as if it were just an 
>> optional feature. But it's not the case that it might be reasonable to 
>> either implement or not implement this in a backend -- it should be 
>> implemented, and those that don't are buggy.
>>
>> I'd be happier with just having an ISEL failure when you try to use fp 
>> atomics on broken targets, rather than adding all this code and 
>> configuration to Clang in order to avoid that. (And, of course, the target 
>> maintainers should also fix them)
>
> +1. I agree with James.
>
> Removing code is often harder than adding it. When you're adding things, 
> you're the only user. Once things are in, they will start growing 
> dependencies that will need to be dealt with if you ever want to remove the 
> code.
>
> Clean solution that works for AMDGPU only for now is better than a 
> potentially permanent workaround.

For amdgpu target, we do need diagnose unsupported atomics (not limited to fp 
atomics) since we do not support libcall due to ISA level linking not 
supported. This is something we cannot fix in a short time and we would rather 
diagnose it than confusing the users with missing symbols in lld.

For other targets, I can make changes to assume fp atomics are supported if 
width is within max inline atomic width of the target. Basically this will let 
fp atomics emitted for these targets and assuming middle end or backend will 
handle them properly.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

In D71726#2536966 , @jyknight wrote:

> My concern is that this is treating a backend _bug_ as if it were just an 
> optional feature. But it's not the case that it might be reasonable to either 
> implement or not implement this in a backend -- it should be implemented, and 
> those that don't are buggy.
>
> I'd be happier with just having an ISEL failure when you try to use fp 
> atomics on broken targets, rather than adding all this code and configuration 
> to Clang in order to avoid that. (And, of course, the target maintainers 
> should also fix them)

+1. I agree with James.

Removing code is often harder than adding it. When you're adding things, you're 
the only user. Once things are in, they will start growing dependencies that 
will need to be dealt with if you ever want to remove the code.

Clean solution that works for AMDGPU only for now is better than a potentially 
permanent workaround.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

My concern is that this is treating a backend _bug_ as if it were just an 
optional feature. But it's not the case that it might be reasonable to either 
implement or not implement this in a backend -- it should be implemented, and 
those that don't are buggy.

I'd be happier with just having an ISEL failure when you try to use fp atomics 
on broken targets, rather than adding all this code and configuration to Clang 
in order to avoid that. (And, of course, the target maintainers should also fix 
them)


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

If the concern is that diagnose fp atomics as unsupported hinders middle end 
and backend work for fixing fp atomic issues, how about adding a 
-fenable-fp-atomics to clang which can override target info about fp atomics 
support.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

This patch focuses on clang work for enabling fp atomics. There is a middle end 
pass for lowering fp atomics to cmpxchg, however not all targets enable it or 
enable it properly. From clang point of view, those targets are not ready to 
say they support fp atomics, therefore it diagnose those situations and let 
clang fail gracefully instead of crashing with isel failure or missing symbols 
in linker.

I have limited resources to work on middle end and backend for all targets. If 
a backend really cares about fp atomics, they should fix the atomic lowering 
pass then enable fp atomics support in clang Target info.

This patch implements fp atomic support in clang. It does not make things worse 
in regarding the bugs in middle ends and backends. I think it is not beneficial 
to blocking this clang change due to middle end and backend issues.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/TargetInfo.h:1479
+Unsupported,
+Init,
+C11LoadStore,

yaxunl wrote:
> rjmccall wrote:
> > `atomic_init` is not actually an atomic operation, so there's never an 
> > inherent reason it can't be supported.
> > 
> > In general, I am torn about this list, because it's simultaneously rather 
> > fine-grained while not seeming nearly fine-grained enough to be truly 
> > general.  What's actually going on on your target?  You have ISA support 
> > for doing some specific operations atomically, but not a general atomic 
> > compare-and-swap operation?  Which means that you then cannot support 
> > support other operations?
> > 
> > It is unfortunate that our layering prevents TargetInfo from simply being 
> > passed the appropriate expression.
> The target hook getAtomicSupport needs an argument for atomic operation. 
> Since not all targets support fp add/sub, we need an enum for add/sub. Since 
> certain release of iOS/macOS does not support C11 load/store, we need an enum 
> for C11 load/store. We could define the enums as {AddSub, C11LoadStore, 
> Other}. However, this would cause a difficulty for emitting diagnostic 
> message for unsupported atomic operations since we map this enum to a string 
> for the atomic operation and use it in the diagnostic message. 'Other' would 
> be mapped to 'other atomic operation' which is not clear what it is.
It's not obviously true that not all targets support FP add/sub, though.  Any 
target that provides compare-and-swap at the width of an FP type can do an 
atomic FP add/sub at that width; it might be less efficient than it would be 
with specific ISA support, but that's true for a lot of atomic operations.  
Surely it's better to just fix whatever bugs LLVM has with lowering atomic FP 
add/sub than to add more abstraction to Clang to handle a special case that 
shouldn't exist.

I don't know what issues Darwin has with C11 load/store; that might be a more 
compelling reason to have this abstraction, although again it seems strange 
that we're outlawing a specific operation when in principle we can just emit it 
less efficiently.



Comment at: clang/lib/Basic/Targets/AArch64.h:143
+}
+  }
 };

yaxunl wrote:
> rjmccall wrote:
> > Why can't targets reliably expand this to an atomic compare-and-exchange if 
> > they support that for the target width?
> There are some bugs in either the middle end or backend causing this not 
> working. For example, half type atomic fadd on amdgcn is not lowered to 
> cmpxchg and the backend has isel failure, bf16 type atomic fadd on arm is not 
> lowered to cmpxchg and the backend has isel failure. The support for each fp 
> type needs to be done case by case. So far there is no target support atomic 
> fadd/sub with half and bf16 type.
Are we legalizing atomicrmw to cmpxchg loops in the backend instead of as LLVM 
IR pass?  That seems like an architectural mistake.  Regardless, this bug 
should just be fixed.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

I still have the same fundamental objection as before to the parts of this 
patch for prohibiting FP add/sub on some targets.

If a particular LLVM target cannot handle transforming an FP add/sub (or any 
other RMW operations!) into the correct cmpxchg or LL/SC loop, that's a bug in 
the backend which should be fixed. I don't see why we ought to add a bunch of 
functionality in the frontend to workaround this?

(Some of the other changes, e.g. to diagnose lack of support for large atomics 
is useful, though.)


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 320477.
yaxunl marked 7 inline comments as done.
yaxunl added a comment.
Herald added a reviewer: jfb.

revised by John's comments


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/AST/Expr.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/AST/Expr.cpp
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/AArch64.h
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/ARM.h
  clang/lib/Basic/Targets/Hexagon.h
  clang/lib/Basic/Targets/Mips.h
  clang/lib/Basic/Targets/OSTargets.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/fp-atomic-ops.c
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/Sema/atomic-requires-library-error.c
  clang/test/SemaCUDA/amdgpu-atomic-ops.cu
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *f, atomic_double *d, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -57,37 +60,38 @@
 
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(f, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+  (int)__opencl_atomic_store(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
 
   int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
   int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(f, 1.0f, memory_order_seq_cst, memory_scope_work_group); // spir-error {{atomic add/sub of '__generic atomic_float' (aka '__generic _Atomic(float)') type requires runtime support that is not available for this target}}
+  __opencl_atomic_fetch_add(d, 1.0, memory_order_seq_cst, memory_scope_work_group); // spir-error {{atomic add/sub of '__generic atomic_double' (aka '__generic _Atomic(double)') type requires runtime support that is not available for this target}}
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, m

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 7 inline comments as done.
yaxunl added inline comments.



Comment at: clang/include/clang/Basic/TargetInfo.h:1478
+  enum class AtomicOperationKind {
+Unsupported,
+Init,

rjmccall wrote:
> This shouldn't be here; if you have places that don't always represent an 
> atomic operation, queries for the kind should return an 
> `Optional` from the classification.
Removed.



Comment at: clang/include/clang/Basic/TargetInfo.h:1479
+Unsupported,
+Init,
+C11LoadStore,

rjmccall wrote:
> `atomic_init` is not actually an atomic operation, so there's never an 
> inherent reason it can't be supported.
> 
> In general, I am torn about this list, because it's simultaneously rather 
> fine-grained while not seeming nearly fine-grained enough to be truly 
> general.  What's actually going on on your target?  You have ISA support for 
> doing some specific operations atomically, but not a general atomic 
> compare-and-swap operation?  Which means that you then cannot support support 
> other operations?
> 
> It is unfortunate that our layering prevents TargetInfo from simply being 
> passed the appropriate expression.
The target hook getAtomicSupport needs an argument for atomic operation. Since 
not all targets support fp add/sub, we need an enum for add/sub. Since certain 
release of iOS/macOS does not support C11 load/store, we need an enum for C11 
load/store. We could define the enums as {AddSub, C11LoadStore, Other}. 
However, this would cause a difficulty for emitting diagnostic message for 
unsupported atomic operations since we map this enum to a string for the atomic 
operation and use it in the diagnostic message. 'Other' would be mapped to 
'other atomic operation' which is not clear what it is.



Comment at: clang/include/clang/Basic/TargetInfo.h:1497
+Unsupported,
+  };
+

rjmccall wrote:
> I think this reflects our current strategies for emitting atomics, but it's a 
> somewhat misleading enum in general because this isn't an exhaustive list of 
> the options — there are certainly possible inline expansions that aren't 
> lock-free.  (For example, you could have an inline spin-lock embedded in the 
> atomic object.)  The goal of this enum is so that TargetInfo only has to have 
> one hook for checking atomic operations?  I would be happier if you included 
> an inline-but-not-lock-free alternative in this enum, even if it's never 
> currently used, so that clients can do the right test.
Added InlineWithLock



Comment at: clang/include/clang/Basic/TargetInfo.h:1501
+  virtual AtomicSupportKind
+  getFPAtomicAddSubSupport(const llvm::fltSemantics &FS) const;
+

rjmccall wrote:
> Why is this needed as a separate hook?
Most target shares getAtomicSupport except FP atomic support, so define a 
virtual function for FP atomic support and let getAtomicSupport call it.



Comment at: clang/lib/AST/ASTContext.cpp:11046
+TargetInfo::AtomicOperationKind
+ASTContext::getTargetAtomicOp(const AtomicExpr *E) const {
+  switch (E->getOp()) {

rjmccall wrote:
> Should this be a method on `AtomicExpr`?  It seems like an intrinsic, 
> target-independent property of the expression.
Yes. moved to AtomicExpr



Comment at: clang/lib/Basic/TargetInfo.cpp:870
+return TargetInfo::AtomicSupportKind::Unsupported;
+  }
+  return AtomicWidthInBits <= AlignmentInBits &&

rjmccall wrote:
> Darwin targets should all be subclasses of `DarwinTargetInfo` in OSTargets.h, 
> so you should be able to just override this there instead of having it in the 
> base case.
done



Comment at: clang/lib/Basic/Targets/AArch64.h:143
+}
+  }
 };

rjmccall wrote:
> Why can't targets reliably expand this to an atomic compare-and-exchange if 
> they support that for the target width?
There are some bugs in either the middle end or backend causing this not 
working. For example, half type atomic fadd on amdgcn is not lowered to cmpxchg 
and the backend has isel failure, bf16 type atomic fadd on arm is not lowered 
to cmpxchg and the backend has isel failure. The support for each fp type needs 
to be done case by case. So far there is no target support atomic fadd/sub with 
half and bf16 type.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: clang/include/clang/Basic/TargetInfo.h:1478
+  enum class AtomicOperationKind {
+Unsupported,
+Init,

This shouldn't be here; if you have places that don't always represent an 
atomic operation, queries for the kind should return an 
`Optional` from the classification.



Comment at: clang/include/clang/Basic/TargetInfo.h:1479
+Unsupported,
+Init,
+C11LoadStore,

`atomic_init` is not actually an atomic operation, so there's never an inherent 
reason it can't be supported.

In general, I am torn about this list, because it's simultaneously rather 
fine-grained while not seeming nearly fine-grained enough to be truly general.  
What's actually going on on your target?  You have ISA support for doing some 
specific operations atomically, but not a general atomic compare-and-swap 
operation?  Which means that you then cannot support support other operations?

It is unfortunate that our layering prevents TargetInfo from simply being 
passed the appropriate expression.



Comment at: clang/include/clang/Basic/TargetInfo.h:1497
+Unsupported,
+  };
+

I think this reflects our current strategies for emitting atomics, but it's a 
somewhat misleading enum in general because this isn't an exhaustive list of 
the options — there are certainly possible inline expansions that aren't 
lock-free.  (For example, you could have an inline spin-lock embedded in the 
atomic object.)  The goal of this enum is so that TargetInfo only has to have 
one hook for checking atomic operations?  I would be happier if you included an 
inline-but-not-lock-free alternative in this enum, even if it's never currently 
used, so that clients can do the right test.



Comment at: clang/include/clang/Basic/TargetInfo.h:1501
+  virtual AtomicSupportKind
+  getFPAtomicAddSubSupport(const llvm::fltSemantics &FS) const;
+

Why is this needed as a separate hook?



Comment at: clang/lib/AST/ASTContext.cpp:11046
+TargetInfo::AtomicOperationKind
+ASTContext::getTargetAtomicOp(const AtomicExpr *E) const {
+  switch (E->getOp()) {

Should this be a method on `AtomicExpr`?  It seems like an intrinsic, 
target-independent property of the expression.



Comment at: clang/lib/Basic/TargetInfo.cpp:870
+return TargetInfo::AtomicSupportKind::Unsupported;
+  }
+  return AtomicWidthInBits <= AlignmentInBits &&

Darwin targets should all be subclasses of `DarwinTargetInfo` in OSTargets.h, 
so you should be able to just override this there instead of having it in the 
base case.



Comment at: clang/lib/Basic/Targets/AArch64.h:143
+}
+  }
 };

Why can't targets reliably expand this to an atomic compare-and-exchange if 
they support that for the target width?


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

@rjmccall I have addressed the comments about diagnostics. Could you please 
review it? Thanks.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

ping


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

ping


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

ping


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D71726#2351069 , @rjmccall wrote:

>> Yes, there are no generically available libcalls for atomic float math -- 
>> but that's okay -- let LLVM handle transform into a cmpxchg loop when 
>> required.
>
> I suspect Yaxun's target cannot provide libcalls at all, which is why he 
> wants to diagnose up-front.  But I agree that we should be thinking about 
> this uniformly, and that his target should be diagnosing *all* unsupported 
> atomics.

amdgpu target currently does not support atomic libcalls. I added a target hook 
for atomic operation support and diagnostics for generic atomic operations by 
John's comments.

Clang has existing diagnostics for unsupported atomic load/store for some 
platforms, and functions about atomic support scattered in target info, AST 
context, and codegen. This change refactors these codes and unify them as a 
target hook.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 307206.
yaxunl edited the summary of this revision.
yaxunl added a comment.

revised by John's comments. Added target hook and diagnostics for generic 
atomic operations.


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/AST/ASTContext.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/AST/ASTContext.cpp
  clang/lib/Basic/TargetInfo.cpp
  clang/lib/Basic/Targets/AArch64.h
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/ARM.h
  clang/lib/Basic/Targets/Hexagon.h
  clang/lib/Basic/Targets/Mips.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/fp-atomic-ops.c
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/Sema/atomic-requires-library-error.c
  clang/test/SemaCUDA/amdgpu-atomic-ops.cu
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *f, atomic_double *d, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -57,37 +60,38 @@
 
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(f, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+  (int)__opencl_atomic_store(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
 
   int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
   int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(f, 1.0f, memory_order_seq_cst, memory_scope_work_group); // spir-error {{atomic add/sub of '__generic atomic_float' (aka '__generic _Atomic(float)') type requires runtime support that is not available for this target}}
+  __opencl_atomic_fetch_add(d, 1.0, memory_order_seq_cst, memory_scope_work_group); // spir-error {{atomic add/sub of '__generic atomic_double' (aka '__generic _Atomic(double)') type requires runtime support that is not available for this target}}
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_and(d,

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

> Yes, there are no generically available libcalls for atomic float math -- but 
> that's okay -- let LLVM handle transform into a cmpxchg loop when required.

I suspect Yaxun's target cannot provide libcalls at all, which is why he wants 
to diagnose up-front.  But I agree that we should be thinking about this 
uniformly, and that his target should be diagnosing *all* unsupported atomics.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D71726#2207700 , @yaxunl wrote:

> clang does not always emit atomic instructions for atomic builtins. Clang may 
> emit lib calls for atomic builtins. Basically clang checks target info about 
> max atomic inline width and if the desired atomic operation exceeds the 
> supported atomic inline width, clang will emit lib calls for atomic builtins. 
> The rationale is that the lib calls may be faster than the IR generated by 
> the LLVM pass. This behavior has long existed and it also applies to fp 
> atomics. I don't think emitting lib calls for atomic builtins is a bug. 
> However, this does introduce the issue about whether the library functions 
> for atomics are available for a specific target. As I said, only the target 
> owners have the answer and therefore I introduced the target hook.

The LLVM AtomicExpandPass is _also_ introducing libcalls (or cmpxchg loops), as 
is appropriate for a given target. We currently, redundantly, support the same 
thing in two places. It's a long-term goal of mine to simplify the atomics code 
in clang, by deferring more of it to LLVM, but some prerequisites (e.g. 
supporting misaligned atomicrmw) are not yet in place. The intent is that it is 
always valid to emit the LLVM atomic IR, and it will be transformed into 
whatever is best on a given target. As such, there's no reason to restrict 
these clang intrinsics.

Yes, there are no generically available libcalls for atomic float math -- but 
that's okay -- let LLVM handle transform into a cmpxchg loop when required.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D71726#2207700 , @yaxunl wrote:

> clang does not always emit atomic instructions for atomic builtins. Clang may 
> emit lib calls for atomic builtins. Basically clang checks target info about 
> max atomic inline width and if the desired atomic operation exceeds the 
> supported atomic inline width, clang will emit lib calls for atomic builtins. 
> The rationale is that the lib calls may be faster than the IR generated by 
> the LLVM pass. This behavior has long existed and it also applies to fp 
> atomics. I don't think emitting lib calls for atomic builtins is a bug. 
> However, this does introduce the issue about whether the library functions 
> for atomics are available for a specific target. As I said, only the target 
> owners have the answer and therefore I introduced the target hook.

If we want the frontend to emit an error when the target doesn't support 
library-based atomics, that seems fine, but there's no reason to only do so for 
floating-point types.  That is, we should have a TargetInfo method that asks 
whether atomics at a given size and alignment are supported at all, similar to 
what we have for "builtin" (lock-free) atomics, and we should check it for all 
the atomic types and operations.

Actually, maybe we should take the existing hook and have it return one of { 
LockFree, Library, Unsupported }.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.
Herald added a subscriber: dexonsmith.

ping


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 284463.
yaxunl marked 2 inline comments as done.
yaxunl added a comment.

Revised by James' comments.


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/AArch64.h
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/ARM.h
  clang/lib/Basic/Targets/Hexagon.h
  clang/lib/Basic/Targets/Mips.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/fp-atomic-ops.c
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaCUDA/amdgpu-atomic-ops.cu
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *f, atomic_double *d, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -57,37 +60,38 @@
 
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(f, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+  (int)__opencl_atomic_store(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
 
   int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
   int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(f, 1.0f, memory_order_seq_cst, memory_scope_work_group); // spir-error {{address argument to atomic operation must be a pointer to atomic integer, pointer or supported floating point type}}
+  __opencl_atomic_fetch_add(d, 1.0, memory_order_seq_cst, memory_scope_work_group); // spir-error {{address argument to atomic operation must be a pointer to atomic integer, pointer or supported floating point type}}
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_and(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 2 inline comments as done.
yaxunl added a comment.

In D71726#2207148 , @jyknight wrote:

> In D71726#2182667 , @tra wrote:
>
>>> If a target would like to treat single and double fp atomics as 
>>> unsupported, it can override the default behavior in its own TargetInfo.
>
> I really don't think this should be a target option at all. Every target can 
> support the atomic fadd/fsub IR instruction (via lowering to a cmpxchg loop 
> if nothing else). If it doesn't work, that's a bug in LLVM. We shouldn't be 
> adding target hooks in Clang to workaround LLVM bugs, rather, we should fix 
> them.
>
> There is one nit -- atomicrmw doesn't (yet) support specifying alignment. 
> There's work now to fix that, but until that's submitted, only 
> naturally-aligned atomicrmw instructions can be created. So, for now, 
> supporting only a naturally-aligned floating-point add would be a reasonable 
> temporary measure.

clang does not always emit atomic instructions for atomic builtins. Clang may 
emit lib calls for atomic builtins. Basically clang checks target info about 
max atomic inline width and if the desired atomic operation exceeds the 
supported atomic inline width, clang will emit lib calls for atomic builtins. 
The rationale is that the lib calls may be faster than the IR generated by the 
LLVM pass. This behavior has long existed and it also applies to fp atomics. I 
don't think emitting lib calls for atomic builtins is a bug. However, this does 
introduce the issue about whether the library functions for atomics are 
available for a specific target. As I said, only the target owners have the 
answer and therefore I introduced the target hook.

>> Do we have sufficient test coverage on all platforms to make sure we're not 
>> generating something that LLVM can't handle everywhere?



> Probably not.

In clang, we only test IR generation, as is done for other atomic builtins. fp 
atomics do not have less coverage compared with other atomic builtins. Actually 
for other atomic builtins we do not even test them on different targets. The 
ISA generation of fp atomics should be done in llvm tests and should not be 
blocking clang change.




Comment at: clang/lib/CodeGen/CGAtomic.cpp:937
+if (Val1.isValid())
+  Val1 = Atomics.convertToAtomicIntPointer(Val1);
+if (Val2.isValid())

jyknight wrote:
> convertToAtomicIntPointer does more than just cast to an int pointer, are you 
> sure the rest is not necessary for fp types?
it is not needed for fp types. If the value type does not match the pointer 
type, clang automatically inserts proper llvm instructions to convert the value 
type to a value type that matches the pointer type. Two codegen tests are added 
(atomic_fetch_add(double*, float) and atomic_fetch_add(double*, int)) to test 
such situations. 



Comment at: clang/lib/Sema/SemaChecking.cpp:4837
 assert(Form != Load);
-if (Form == Init || (Form == Arithmetic && ValType->isIntegerType()))
+if (Form == Init || (Form == Arithmetic && ValType->isIntegerType()) ||
+(IsAddSub && ValType->isFloatingType()))

jyknight wrote:
> This is confusing, and took me a bit to understand what you're doing. I'd 
> suggest reordering the clauses, putting the pointer case first, e.g.:
> ```
> if (Form == Arithmetic && ValType->isPointerType())
>   Ty = Context.getPointerDiffType();
> else if (Form == Init || Form == Arithmetic)
>   Ty = ValType;
> else if (Form == Copy || Form == Xchg) .
> else ..
> ...
> ```
done


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Oh, one more note, C11 has -- and clang already supports -- `_Atomic long 
double x; x += 4;` via lowering to a cmpxchg loop. Now that we have an LLVM IR 
representation for atomicrmw fadd/fsub, clang should be lowering the _Atomic += 
to that, too. (Doesn't need to be in this patch, but it should be done.)


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D71726#2182667 , @tra wrote:

>> If a target would like to treat single and double fp atomics as unsupported, 
>> it can override the default behavior in its own TargetInfo.

I really don't think this should be a target option at all. Every target can 
support the atomic fadd/fsub IR instruction (via lowering to a cmpxchg loop if 
nothing else). If it doesn't work, that's a bug in LLVM. We shouldn't be adding 
target hooks in Clang to workaround LLVM bugs, rather, we should fix them.

There is one nit -- atomicrmw doesn't (yet) support specifying alignment. 
There's work now to fix that, but until that's submitted, only 
naturally-aligned atomicrmw instructions can be created. So, for now, 
supporting only a naturally-aligned floating-point add would be a reasonable 
temporary measure.

> Do we have sufficient test coverage on all platforms to make sure we're not 
> generating something that LLVM can't handle everywhere?

Probably not.

> If not, perhaps we should default to unsupported and only enable it for known 
> working targets.

No, I don't think that's a good way to go. We should fix LLVM if it's broken.




Comment at: clang/lib/CodeGen/CGAtomic.cpp:937
+if (Val1.isValid())
+  Val1 = Atomics.convertToAtomicIntPointer(Val1);
+if (Val2.isValid())

convertToAtomicIntPointer does more than just cast to an int pointer, are you 
sure the rest is not necessary for fp types?



Comment at: clang/lib/Sema/SemaChecking.cpp:4837
 assert(Form != Load);
-if (Form == Init || (Form == Arithmetic && ValType->isIntegerType()))
+if (Form == Init || (Form == Arithmetic && ValType->isIntegerType()) ||
+(IsAddSub && ValType->isFloatingType()))

This is confusing, and took me a bit to understand what you're doing. I'd 
suggest reordering the clauses, putting the pointer case first, e.g.:
```
if (Form == Arithmetic && ValType->isPointerType())
  Ty = Context.getPointerDiffType();
else if (Form == Init || Form == Arithmetic)
  Ty = ValType;
else if (Form == Copy || Form == Xchg) .
else ..
...
```


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

ping


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D71726#2182667 , @tra wrote:

> LGTM, modulo couple of nits.
>
> @jyknight are you OK with this?
>
> In D71726#2179428 , @yaxunl wrote:
>
>> Make IEEE single and double type as supported for fp atomics in all targets 
>> by default. This is based on the assumption that AtomicExpandPass or its 
>> ongoing work is sufficient to support fp atomics for all targets. This is to 
>> facilitate middle end and backend end development to support fp atomics.
>>
>> If a target would like to treat single and double fp atomics as unsupported, 
>> it can override the default behavior in its own TargetInfo.
>
> Do we have sufficient test coverage on all platforms to make sure we're not 
> generating something that LLVM can't handle everywhere?
> If not, perhaps we should default to unsupported and only enable it for known 
> working targets.

I updated TargetInfo for fp atomic support for common targets. Basically by 
default fp atomic support is now off. It is enabled only for targets which do 
not generate lib calls for fp atomics. This is because the availability of lib 
call depends on platform, so it is up to the Target owners to determine whether 
the support is available if lib call is needed. For those targets which are 
able to generate llvm fp atomic instructions, fp atomic support is enabled in 
clang, and tests are added to cover them.




Comment at: clang/lib/CodeGen/CGAtomic.cpp:889-891
+if (MemTy->isFloatingType()) {
+  ShouldCastToIntPtrTy = false;
+}

tra wrote:
> `ShouldCastToIntPtrTy = !MemTy->isFloatingType();`
done



Comment at: clang/test/Sema/atomic-ops.c:102-103
 void f(_Atomic(int) *i, const _Atomic(int) *ci,
-   _Atomic(int*) *p, _Atomic(float) *d,
+   _Atomic(int*) *p, _Atomic(float) *d, _Atomic(double) *d2,
+   _Atomic(long double) *d3,
int *I, const int *CI,

tra wrote:
> Rename arguments? d -> f, d2 -> d, d3 -> ld ?
done


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 283067.
yaxunl added a comment.
Herald added subscribers: atanasyan, sdardis.

added tests for targets supporting fp atomics.


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/AArch64.h
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/Basic/Targets/ARM.h
  clang/lib/Basic/Targets/Hexagon.h
  clang/lib/Basic/Targets/Mips.h
  clang/lib/Basic/Targets/X86.h
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/fp-atomic-ops.c
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaCUDA/amdgpu-atomic-ops.cu
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *f, atomic_double *d, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -57,37 +60,38 @@
 
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(f, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+  (int)__opencl_atomic_store(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
 
   int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
   int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(f, 1.0f, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_fetch_add(d, 1.0, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_and(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
 
   __opencl_atomic_fetch_min(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __op

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 283028.
yaxunl marked 2 inline comments as done.
yaxunl added a comment.

revised by Artem's comments.


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaCUDA/amdgpu-atomic-ops.cu
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *f, atomic_double *d, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -57,37 +60,38 @@
 
   __opencl_atomic_load(i, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(p, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_load(d, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_load(f, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(ci, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_load(i_c, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to non-constant _Atomic type ('__constant atomic_int *' (aka '__constant _Atomic(int) *') invalid)}}
 
   __opencl_atomic_store(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_store(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  (int)__opencl_atomic_store(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
+  (int)__opencl_atomic_store(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{operand of type 'void' where arithmetic or pointer type is required}}
 
   int exchange_1 = __opencl_atomic_exchange(i, 1, memory_order_seq_cst, memory_scope_work_group);
   int exchange_2 = __opencl_atomic_exchange(I, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to _Atomic}}
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(f, 1.0f, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_fetch_add(d, 1.0, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_and(f, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
 
   __opencl_atomic_fetch_min(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_max(i, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_min(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic 

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Artem Belevich via Phabricator via cfe-commits]

tra added a comment.

LGTM, modulo couple of nits.

@jyknight are you OK with this?

In D71726#2179428 , @yaxunl wrote:

> Make IEEE single and double type as supported for fp atomics in all targets 
> by default. This is based on the assumption that AtomicExpandPass or its 
> ongoing work is sufficient to support fp atomics for all targets. This is to 
> facilitate middle end and backend end development to support fp atomics.
>
> If a target would like to treat single and double fp atomics as unsupported, 
> it can override the default behavior in its own TargetInfo.

Do we have sufficient test coverage on all platforms to make sure we're not 
generating something that LLVM can't handle everywhere?
If not, perhaps we should default to unsupported and only enable it for known 
working targets.




Comment at: clang/lib/CodeGen/CGAtomic.cpp:889-891
+if (MemTy->isFloatingType()) {
+  ShouldCastToIntPtrTy = false;
+}

`ShouldCastToIntPtrTy = !MemTy->isFloatingType();`



Comment at: clang/test/Sema/atomic-ops.c:102-103
 void f(_Atomic(int) *i, const _Atomic(int) *ci,
-   _Atomic(int*) *p, _Atomic(float) *d,
+   _Atomic(int*) *p, _Atomic(float) *d, _Atomic(double) *d2,
+   _Atomic(long double) *d3,
int *I, const int *CI,

Rename arguments? d -> f, d2 -> d, d3 -> ld ?


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

ping. I think I have addressed all the issues in FE. I think issues in 
AtomicExpandPass should be addressed by separate patches. Can we land this? 
Thanks.


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

https://reviews.llvm.org/D71726

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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 281296.
yaxunl added a comment.

Make IEEE single and double type as supported for fp atomics in all targets by 
default. This is based on the assumption that AtomicExpandPass or its ongoing 
work is sufficient to support fp atomics for all targets. This is to facilitate 
middle end and backend end development to support fp atomics.

If a target would like to treat single and double fp atomics as unsupported, it 
can override the default behavior in its own TargetInfo.


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaCUDA/amdgpu-atomic-ops.cu
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *d, atomic_double *d2, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -70,7 +73,8 @@
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(d, 1.0f, memory_order_seq_cst, memory_scope_work_group);
+  __opencl_atomic_fetch_add(d2, 1.0, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
Index: clang/test/SemaCUDA/amdgpu-atomic-ops.cu
===
--- /dev/null
+++ clang/test/SemaCUDA/amdgpu-atomic-ops.cu
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=amdgcn-amd-amdhsa \
+// RUN:   -fcuda-is-device -target-cpu gfx906 -fnative-half-type \
+// RUN:   -fnative-half-arguments-and-returns
+
+// REQUIRES: amdgpu-registered-target
+
+#include "Inputs/cuda.h"
+#include 
+
+__device__ _Float16 test_Flot16(_Float16 *p) {
+  return __atomic_fetch_sub(p, 1.0f16, memory_order_relaxed);
+  // expected-error@-1 {{address argument to atomic operation must be a pointer to integer, pointer or supported floating point type ('_Float16 *' invalid)}}
+}
+
+__device__ __fp16 test_fp16(__fp16 *p) {
+  return __atomic_fetch_sub(p, 1.0f16, memory_order_relaxed);
+  // expected-error@-1 {{address argument to atomic operation must be a pointer to integer, pointer or supported floating point type ('__fp16 *' invalid)}}
+}
Index: clang/test/Sema/atomic-ops.c
===
--- clang/test/Sema/atomic-ops.c
+++ clang/test/Sema/atomic-ops.c
@@ -99,7 +99,8 @@
 #define _AS2 __attribute__((address_space(2)))
 
 void f(_Atomic(int) *i, const _Atomic(int) *ci,
-   _Atomic(int*) *p, _Atomic(float) *d,
+   _Atomic(int*) *p, _Atomic(float) *d, _Atomic(double) *d2,
+   _Atomic(long double) *d3,
int *I, const int *CI,
int **P, float *D, struct S *s1, struct S *s2) {
   __c11_atomic_init(I, 5); // expected-error {{pointer to _Atomic}}
@@ -166,13 +167,15 @@
 
   __c11_atomic_fetch_add(i, 1, memory_order_se

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D71726#2165494 , @jyknight wrote:

> In D71726#2165445 , @yaxunl wrote:
>
> > In D71726#2165424 , @jyknight 
> > wrote:
> >
> > > Why not have clang always emit atomicrmw for floats, and let 
> > > AtomicExpandPass handle legalizing that into integer atomics if 
> > > necessary, rather than adding a target hook in clang?
> >
> >
> > Not all targets can legalize fp atomics by AtomicExpandPass. Some targets 
> > need library support.
>
>
> That isn't true, because you can do so generically with a cmpxchg loop, 
> assuming that size of atomic is supported by the target. This might not be 
> the most efficient lowering choice, but it's always possible as a fallback. 
> (And if the size is too large, then AtomicExpandPass will lower the cmpxchg 
> to the libatomic call.)
>
> If a target wants to tell AtomicExpandPass that fp add/sub are supported, and 
> then lower the resulting ATOMIC_LOAD_FSUB sdag node into a libcall of its 
> choice, that's also ok (as long as the libcall is lock-free).


how about other fp types e.g. bf16, half, long double? Do we need to diagnose 
them or not?


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

In D71726#2165445 , @yaxunl wrote:

> In D71726#2165424 , @jyknight wrote:
>
> > Why not have clang always emit atomicrmw for floats, and let 
> > AtomicExpandPass handle legalizing that into integer atomics if necessary, 
> > rather than adding a target hook in clang?
>
>
> Not all targets can legalize fp atomics by AtomicExpandPass. Some targets 
> need library support.


That isn't true, because you can do so generically with a cmpxchg loop, 
assuming that size of atomic is supported by the target. This might not be the 
most efficient lowering choice, but it's always possible as a fallback. (And if 
the size is too large, then AtomicExpandPass will lower the cmpxchg to the 
libatomic call.)

If a target wants to tell AtomicExpandPass that fp add/sub are supported, and 
then lower the resulting ATOMIC_LOAD_FSUB sdag node into a libcall of its 
choice, that's also ok (as long as the libcall is lock-free).


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D71726#2165445 , @yaxunl wrote:

> In D71726#2165424 , @jyknight wrote:
>
> > Why not have clang always emit atomicrmw for floats, and let 
> > AtomicExpandPass handle legalizing that into integer atomics if necessary, 
> > rather than adding a target hook in clang?
>
>
> Not all targets can legalize fp atomics by AtomicExpandPass. Some targets 
> need library support.


What are they missing? It can be expanded to a cmpxchg loop with bitcast to an 
integer type of the same size.


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D71726#2165424 , @jyknight wrote:

> Why not have clang always emit atomicrmw for floats, and let AtomicExpandPass 
> handle legalizing that into integer atomics if necessary, rather than adding 
> a target hook in clang?


Not all targets can legalize fp atomics by AtomicExpandPass. Some targets need 
library support.


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[James Y Knight via Phabricator via cfe-commits]

jyknight added a comment.

Why not have clang always emit atomicrmw for floats, and let AtomicExpandPass 
handle legalizing that into integer atomics if necessary, rather than adding a 
target hook in clang?


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 279032.
yaxunl marked 2 inline comments as done.
yaxunl added a comment.

use llvm::fltSemantics for checking


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaCUDA/amdgpu-atomic-ops.cu
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *d, atomic_double *d2, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -70,7 +73,8 @@
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(d, 1.0f, memory_order_seq_cst, memory_scope_work_group); // spir-error {{address argument to atomic operation must be a pointer to atomic integer, pointer or supported floating point type ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(d2, 1.0, memory_order_seq_cst, memory_scope_work_group); // spir-error {{address argument to atomic operation must be a pointer to atomic integer, pointer or supported floating point type ('__generic atomic_double *' (aka '__generic _Atomic(double) *') invalid)}}
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
Index: clang/test/SemaCUDA/amdgpu-atomic-ops.cu
===
--- /dev/null
+++ clang/test/SemaCUDA/amdgpu-atomic-ops.cu
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=amdgcn-amd-amdhsa \
+// RUN:   -fcuda-is-device -target-cpu gfx906 -fnative-half-type \
+// RUN:   -fnative-half-arguments-and-returns
+
+// REQUIRES: amdgpu-registered-target
+
+#include "Inputs/cuda.h"
+#include 
+
+__device__ _Float16 test_Flot16(_Float16 *p) {
+  return __atomic_fetch_sub(p, 1.0f16, memory_order_relaxed);
+  // expected-error@-1 {{address argument to atomic operation must be a pointer to integer, pointer or supported floating point type ('_Float16 *' invalid)}}
+}
+
+__device__ __fp16 test_fp16(__fp16 *p) {
+  return __atomic_fetch_sub(p, 1.0f16, memory_order_relaxed);
+  // expected-error@-1 {{address argument to atomic operation must be a pointer to integer, pointer or supported floating point type ('__fp16 *' invalid)}}
+}
Index: clang/test/Sema/atomic-ops.c
===
--- clang/test/Sema/atomic-ops.c
+++ clang/test/Sema/atomic-ops.c
@@ -99,7 +99,8 @@
 #define _AS2 __attribute__((address_space(2)))
 
 void f(_Atomic(int) *i, const _Atomic(int) *ci,
-   _Atomic(int*) *p, _Atomic(float) *d,
+   _Atomic(int*) *p, _Atomic(float) *d, _Atomic(double) *d2,
+   _Atomic(long double) *d3,
int *I, const int *CI,
int **P, float *D, struct S *s1, struct S *s2) {
   __c11_atomic_init(I, 5); // expected-error {{pointer t

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 4 inline comments as done.
yaxunl added inline comments.



Comment at: clang/include/clang/Basic/TargetInfo.h:1418
+  /// Whether floating point atomic fetch add/sub is supported.
+  virtual bool isFPAtomicFetchAddSubSupported() const { return false; }
+

tra wrote:
> yaxunl wrote:
> > tra wrote:
> > > I think it should be predicated on specific type.
> > > E.g. NVPTX supports atomic ops on fp32 ~everywhere, but fp64 atomic 
> > > add/sub is only supported on newer GPUs.
> > > And then there's fp16...
> > will do and add tests for fp16
> The number of bits alone may not be sufficient to differentiate the FP 
> variants.
> E.g. 16-bit floats currently have 2 variants: IEEE FP16 and BFloat16 
> (supported by intel and newer NVIDIA GPUs).
> CUDA-11 has introduced TF32 FP format, so we're likely to have more than one 
> 32-bit FP type, too.
> I think PPC has an odd `long double` variant represented as pair of 64-bit 
> doubles.
> 
will use llvm::fltSemantics for checking, which should cover different fp types.



Comment at: clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu:26
+  // CHECK: atomicrmw fsub double* {{.*}} monotonic
+  return __atomic_fetch_sub(p, 1.0, memory_order_relaxed);
+}

ldionne wrote:
> Nitpick, but this should be `1.0L` to be consistent.
done


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: clang/include/clang/Basic/TargetInfo.h:1418
+  /// Whether floating point atomic fetch add/sub is supported.
+  virtual bool isFPAtomicFetchAddSubSupported() const { return false; }
+

yaxunl wrote:
> tra wrote:
> > I think it should be predicated on specific type.
> > E.g. NVPTX supports atomic ops on fp32 ~everywhere, but fp64 atomic add/sub 
> > is only supported on newer GPUs.
> > And then there's fp16...
> will do and add tests for fp16
The number of bits alone may not be sufficient to differentiate the FP variants.
E.g. 16-bit floats currently have 2 variants: IEEE FP16 and BFloat16 (supported 
by intel and newer NVIDIA GPUs).
CUDA-11 has introduced TF32 FP format, so we're likely to have more than one 
32-bit FP type, too.
I think PPC has an odd `long double` variant represented as pair of 64-bit 
doubles.



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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu:26
+  // CHECK: atomicrmw fsub double* {{.*}} monotonic
+  return __atomic_fetch_sub(p, 1.0, memory_order_relaxed);
+}

Nitpick, but this should be `1.0L` to be consistent.


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 265606.
yaxunl marked 2 inline comments as done.
yaxunl edited the summary of this revision.
yaxunl added a reviewer: tra.
yaxunl added a comment.

check supported fp atomics by bits.


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaCUDA/amdgpu-atomic-ops.cu
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,10 +1,13 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
 #pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
 #pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_fp16 : enable
 
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
@@ -36,7 +39,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *d, atomic_double *d2, atomic_half *h, // expected-error {{unknown type name 'atomic_half'}}
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -70,7 +73,8 @@
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(d, 1.0f, memory_order_seq_cst, memory_scope_work_group); // spir-error {{address argument to atomic operation must be a pointer to atomic integer, pointer or supported floating point type ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(d2, 1.0, memory_order_seq_cst, memory_scope_work_group); // spir-error {{address argument to atomic operation must be a pointer to atomic integer, pointer or supported floating point type ('__generic atomic_double *' (aka '__generic _Atomic(double) *') invalid)}}
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
Index: clang/test/SemaCUDA/amdgpu-atomic-ops.cu
===
--- /dev/null
+++ clang/test/SemaCUDA/amdgpu-atomic-ops.cu
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 %s -verify -fsyntax-only -triple=amdgcn-amd-amdhsa \
+// RUN:   -fcuda-is-device -target-cpu gfx906 -fnative-half-type \
+// RUN:   -fnative-half-arguments-and-returns
+
+// REQUIRES: amdgpu-registered-target
+
+#include "Inputs/cuda.h"
+#include 
+
+__device__ _Float16 test_Flot16(_Float16 *p) {
+  return __atomic_fetch_sub(p, 1.0f16, memory_order_relaxed);
+  // expected-error@-1 {{address argument to atomic operation must be a pointer to integer, pointer or supported floating point type ('_Float16 *' invalid)}}
+}
+
+__device__ __fp16 test_fp16(__fp16 *p) {
+  return __atomic_fetch_sub(p, 1.0f16, memory_order_relaxed);
+  // expected-error@-1 {{address argument to atomic operation must be a pointer to integer, pointer or supported floating point type ('__fp16 *' invalid)}}
+}
Index: clang/test/Sema/atomic-ops.c
===
--- clang/test/Sema/atomic-ops.c
+++ clang/test/Sema/atomic-ops.c
@@ -99,7 +99,8 @@
 #define _AS2 __attribute__((address_space(2)))
 
 void f(_Atomic(int) *i, const _Atomic(int) *ci,
-   _Atomic(int*) *p, _Atomic(float) *d,
+   _Atomic(int*) *p, _Atomic(float) *d, _Atomic(double) *d2,
+   _Atomic(long double) *d3,
int *I, const int *CI,
int **P, float *D, struct S *s1,

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 3 inline comments as done.
yaxunl added inline comments.



Comment at: clang/include/clang/Basic/TargetInfo.h:1418
+  /// Whether floating point atomic fetch add/sub is supported.
+  virtual bool isFPAtomicFetchAddSubSupported() const { return false; }
+

tra wrote:
> I think it should be predicated on specific type.
> E.g. NVPTX supports atomic ops on fp32 ~everywhere, but fp64 atomic add/sub 
> is only supported on newer GPUs.
> And then there's fp16...
will do and add tests for fp16



Comment at: clang/test/CodeGen/atomic-ops.c:296
+  // CHECK: fsub
+  return __atomic_sub_fetch(p, 1.0, memory_order_relaxed);
+}

ldionne wrote:
> yaxunl wrote:
> > ldionne wrote:
> > > Sorry if that's a dumb question, but I'm a bit confused: `p` is  a 
> > > `float*`, but then we add a double `1.0` to it. Is that intended, or 
> > > should that be `double *p` instead (or `1.0f`)?
> > In this case, the value type is converted to the pointee type of the 
> > pointer operand.
> Ok, thanks for the clarification. Yeah, it was a dumb question after all. I 
> still think it should be made clearer by using `1.0f`.
this test has been removed. the new tests do not have this issue.


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: clang/test/CodeGen/atomic-ops.c:296
+  // CHECK: fsub
+  return __atomic_sub_fetch(p, 1.0, memory_order_relaxed);
+}

yaxunl wrote:
> ldionne wrote:
> > Sorry if that's a dumb question, but I'm a bit confused: `p` is  a 
> > `float*`, but then we add a double `1.0` to it. Is that intended, or should 
> > that be `double *p` instead (or `1.0f`)?
> In this case, the value type is converted to the pointee type of the pointer 
> operand.
Ok, thanks for the clarification. Yeah, it was a dumb question after all. I 
still think it should be made clearer by using `1.0f`.


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Artem Belevich via Phabricator via cfe-commits]

tra added inline comments.



Comment at: clang/include/clang/Basic/TargetInfo.h:1418
+  /// Whether floating point atomic fetch add/sub is supported.
+  virtual bool isFPAtomicFetchAddSubSupported() const { return false; }
+

I think it should be predicated on specific type.
E.g. NVPTX supports atomic ops on fp32 ~everywhere, but fp64 atomic add/sub is 
only supported on newer GPUs.
And then there's fp16...


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 265479.
yaxunl marked an inline comment as done.
yaxunl edited the summary of this revision.
yaxunl added a comment.
Herald added subscribers: kerbowa, nhaehnle, jvesely.

Added TargetInfo::isFPAtomicFetchAddSubSupported to guard fp atomic.


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/TargetInfo.h
  clang/lib/Basic/Targets/AMDGPU.h
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGenCUDA/amdgpu-atomic-ops.cu
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -1,5 +1,7 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=spir64
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only -triple=amdgcn-amdhsa-amd-opencl
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify=expected,spir \
+// RUN:   -fsyntax-only -triple=spir64
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -verify -fsyntax-only \
+// RUN:   -triple=amdgcn-amd-amdhsa
 
 // Basic parsing/Sema tests for __opencl_atomic_*
 
@@ -36,7 +38,7 @@
 
 atomic_int gn;
 void f(atomic_int *i, const atomic_int *ci,
-   atomic_intptr_t *p, atomic_float *d,
+   atomic_intptr_t *p, atomic_float *d, atomic_double *d2,
int *I, const int *CI,
intptr_t *P, float *D, struct S *s1, struct S *s2,
global atomic_int *i_g, local atomic_int *i_l, private atomic_int *i_p,
@@ -70,7 +72,8 @@
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // spir-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(d2, 1, memory_order_seq_cst, memory_scope_work_group); // spir-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_double *' (aka '__generic _Atomic(double) *') invalid)}}
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
Index: clang/test/CodeGenOpenCL/atomic-ops.cl
===
--- clang/test/CodeGenOpenCL/atomic-ops.cl
+++ clang/test/CodeGenOpenCL/atomic-ops.cl
@@ -1,12 +1,17 @@
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -emit-llvm -O0 -o - -triple=amdgcn-amd-amdhsa-amdgizcl | opt -instnamer -S | FileCheck %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -emit-llvm -O0 -o - -triple=amdgcn-amd-amdhsa \
+// RUN:   | opt -instnamer -S | FileCheck %s
 
 // Also test serialization of atomic operations here, to avoid duplicating the test.
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -emit-pch -O0 -o %t -triple=amdgcn-amd-amdhsa-amdgizcl
-// RUN: %clang_cc1 %s -cl-std=CL2.0 -include-pch %t -O0 -triple=amdgcn-amd-amdhsa-amdgizcl -emit-llvm -o - | opt -instnamer -S | FileCheck %s
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -emit-pch -O0 -o %t -triple=amdgcn-amd-amdhsa
+// RUN: %clang_cc1 %s -cl-std=CL2.0 -include-pch %t -O0 -triple=amdgcn-amd-amdhsa \
+// RUN:   -emit-llvm -o - | opt -instnamer -S | FileCheck %s
 
 #ifndef ALREADY_INCLUDED
 #define ALREADY_INCLUDED
 
+#pragma OPENCL EXTENSION cl_khr_int64_base_atomics : enable
+#pragma OPENCL EXTENSION cl_khr_int64_extended_atomics : enable
+
 typedef __INTPTR_TYPE__ intptr_t;
 typedef int int8 __attribute__((ext_vector_type(8)));
 
@@ -185,6 +190,18 @@
   return __opencl_atomic_exchange(d, 2, memory_order_seq_cst, memory_scope_work_group);
 }
 
+float ff4(global atomic_float *d, float a) {
+  // CHECK-LABEL: @ff4
+  // CHECK: atomicrmw fadd float addrspace(1)* {{.*}} syncscope("workgroup-one-as") monotonic
+  return __opencl_atomic_fetch_add(d, a, memory_order_relaxed, memory_scope_work_group);
+}
+
+float ff5(global atomic_double *d, double a) {
+  // CHECK-LABEL: @ff5
+  // CHECK: atomicrmw fadd double addrspace(1)* {{.*}} syncscope("workgroup-one-as") monotonic
+  return __opencl_atomic_fetch_add(d, a, memory_order_relaxed, memory_scope_w

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[JF Bastien via Phabricator via cfe-commits]

jfb added a comment.

In D71726#2047566 , @ldionne wrote:

> In D71726#1791904 , @jfb wrote:
>
> > This generally seems fine. Does it work on most backends? I want to make 
> > sure it doesn't fail in backends :)
> >
> > Also, @ldionne / @EricWF / @mclow.lists do you need this in libc++ for 
> > floating-point atomic support?
>
>
> Yes, I guess we do in order to implement `fetch_add` & friends on floating 
> point types (https://wg21.link/P0020R6).
>
> The builtins would need to work on `float`, `double` and `long double`. The 
> code seems to suggest it does, however the tests only check for `float`. Does 
> this support `__atomic_fetch_{add,sub}` on `double` and `long double`?


libc++ could implement `atomic` using a cmpxchg loop with `bit_cast` and 
the FP instruction in most cases, and only use these builtins if available.


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 3 inline comments as done.
yaxunl added a comment.

In D71726#2047566 , @ldionne wrote:

> In D71726#1791904 , @jfb wrote:
>
> > This generally seems fine. Does it work on most backends? I want to make 
> > sure it doesn't fail in backends :)
> >
> > Also, @ldionne / @EricWF / @mclow.lists do you need this in libc++ for 
> > floating-point atomic support?
>
>
> Yes, I guess we do in order to implement `fetch_add` & friends on floating 
> point types (https://wg21.link/P0020R6).
>
> The builtins would need to work on `float`, `double` and `long double`. The 
> code seems to suggest it does, however the tests only check for `float`. Does 
> this support `__atomic_fetch_{add,sub}` on `double` and `long double`?


It depends on target. For x86_64, `__atomic_fetch_{add,sub}` on `double` and 
`long double` are translated to `__atomic_fetch_sub_8` and 
`__atomic_fetch_sub_16`.
For amdgcn, `__atomic_fetch_{add,sub}` on `double` is translated to fp atomic 
insts. `long double` is the same as `double` on amdgcn.




Comment at: clang/test/CodeGen/atomic-ops.c:296
+  // CHECK: fsub
+  return __atomic_sub_fetch(p, 1.0, memory_order_relaxed);
+}

ldionne wrote:
> Sorry if that's a dumb question, but I'm a bit confused: `p` is  a `float*`, 
> but then we add a double `1.0` to it. Is that intended, or should that be 
> `double *p` instead (or `1.0f`)?
In this case, the value type is converted to the pointee type of the pointer 
operand.


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Louis Dionne via Phabricator via cfe-commits]

ldionne added a comment.

In D71726#1791904 , @jfb wrote:

> This generally seems fine. Does it work on most backends? I want to make sure 
> it doesn't fail in backends :)
>
> Also, @ldionne / @EricWF / @mclow.lists do you need this in libc++ for 
> floating-point atomic support?


Yes, I guess we do in order to implement `fetch_add` & friends on floating 
point types (https://wg21.link/P0020R6).

The builtins would need to work on `float`, `double` and `long double`. The 
code seems to suggest it does, however the tests only check for `float`. Does 
this support `__atomic_fetch_{add,sub}` on `double` and `long double`?


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Louis Dionne via Phabricator via cfe-commits]

ldionne added inline comments.



Comment at: clang/test/CodeGen/atomic-ops.c:296
+  // CHECK: fsub
+  return __atomic_sub_fetch(p, 1.0, memory_order_relaxed);
+}

Sorry if that's a dumb question, but I'm a bit confused: `p` is  a `float*`, 
but then we add a double `1.0` to it. Is that intended, or should that be 
`double *p` instead (or `1.0f`)?


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 2 inline comments as done.
yaxunl added a comment.

In D71726#2039319 , @arsenm wrote:

> In D71726#1801346 , @__simt__ wrote:
>
> > In D71726#1792852 , @yaxunl wrote:
> >
> > > In D71726#1791904 , @jfb wrote:
> > >
> > > > This generally seems fine. Does it work on most backends? I want to 
> > > > make sure it doesn't fail in backends :)
> > >
> > >
> > > For x86_64, amdgcn, aarch64, armv7, mips64, it is translated to cmpxchg 
> > > by AtomicExpandPass and backends did codegen successfully.
> > >
> > > For hexagon, riscv32, it is translated to call of `__atomic_fetch_add_4` 
> > > for fadd float. This is concerning. Probably we need to add 
> > > `__atomic_fetch_{add|sub}_{f16|f32|f64}` ?
> >
> >
> > For systems that have load-link/store-conditional architectures, like ARM / 
> > PPC / base RISC-V without extension, I would imagine that using a cmpxchg 
> > loop is much worse than simply doing the floating-point add/sub in the 
> > middle of the atomic mini-transaction. I'm sure that we want back-ends to 
> > be capable of implementing this better than what this pass is doing, even 
> > when they don't have "native" fp atomics.
> >
> > You listed amdgcn... what does this do on nvptx?
>
>
> Targets can implement shouldExpandAtomicRMWInIR for the desired behavior, 
> which NVPTX currently does not implement. Looking at AtomicExpandPass, it 
> looks like either cmpxchg or LLSC expansions should work for the FP atomics 
> already


nvptx is similar to hexagon and riscv32, where fp atomics is translated to call 
of __atomic_fetch_add_4.

Since currently only amdgcn supports fp atomics, I am going to add a TargetInfo 
hook about whether fp atomics is supported and only emit fp atomics for targets 
supporting it.




Comment at: clang/lib/CodeGen/CGAtomic.cpp:597-598
   case AtomicExpr::AO__atomic_add_fetch:
-PostOp = llvm::Instruction::Add;
+PostOp = E->getValueType()->isFloatingType() ? llvm::Instruction::FAdd
+ : llvm::Instruction::Add;
 LLVM_FALLTHROUGH;

arsenm wrote:
> Should this really be based on the type, or should the builtin name be 
> different for FP?
I think the original name is better. They are exactly what they are intended to 
be. They were not able to handle fp types therefore they used to emit 
diagnostics when fp types were passed to them. However now they are able to 
handle fp types.


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 265325.
yaxunl added a reviewer: arsenm.
yaxunl added a comment.
Herald added a subscriber: wdng.

rebase


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

https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/atomic-ops.c
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -70,7 +70,7 @@
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
Index: clang/test/Sema/atomic-ops.c
===
--- clang/test/Sema/atomic-ops.c
+++ clang/test/Sema/atomic-ops.c
@@ -166,13 +166,13 @@
 
   __c11_atomic_fetch_add(i, 1, memory_order_seq_cst);
   __c11_atomic_fetch_add(p, 1, memory_order_seq_cst);
-  __c11_atomic_fetch_add(d, 1, memory_order_seq_cst); // expected-error {{must be a pointer to atomic integer or pointer}}
+  __c11_atomic_fetch_add(d, 1, memory_order_seq_cst);
 
-  __atomic_fetch_add(i, 3, memory_order_seq_cst); // expected-error {{pointer to integer or pointer}}
+  __atomic_fetch_add(i, 3, memory_order_seq_cst); // expected-error {{pointer to integer, pointer or floating point type}}
   __atomic_fetch_sub(I, 3, memory_order_seq_cst);
   __atomic_fetch_sub(P, 3, memory_order_seq_cst);
-  __atomic_fetch_sub(D, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
-  __atomic_fetch_sub(s1, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
+  __atomic_fetch_sub(D, 3, memory_order_seq_cst);
+  __atomic_fetch_sub(s1, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer, pointer or floating point type}}
   __atomic_fetch_min(D, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer}}
   __atomic_fetch_max(P, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer}}
   __atomic_fetch_max(p, 3);   // expected-error {{too few arguments to function call, expected 3, have 2}}
Index: clang/test/CodeGenOpenCL/atomic-ops.cl
===
--- clang/test/CodeGenOpenCL/atomic-ops.cl
+++ clang/test/CodeGenOpenCL/atomic-ops.cl
@@ -185,6 +185,12 @@
   return __opencl_atomic_exchange(d, 2, memory_order_seq_cst, memory_scope_work_group);
 }
 
+float ff4(global atomic_float *d, float a) {
+  // CHECK-LABEL: @ff4
+  // CHECK: atomicrmw fadd float addrspace(1)* {{.*}} syncscope("workgroup-one-as") monotonic
+  return __opencl_atomic_fetch_add(d, a, memory_order_relaxed, memory_scope_work_group);
+}
+
 // CHECK-LABEL: @atomic_init_foo
 void atomic_init_foo()
 {
Index: clang/test/CodeGen/atomic-ops.c
===
--- clang/test/CodeGen/atomic-ops.c
+++ clang/test/CodeGen/atomic-ops.c
@@ -283,6 +283,19 @@
   return __c11_atomic_fetch_add(p, 1, memory_order_relaxed);
 }
 
+float ffp1(_Atomic(float) *p) {
+  // CHECK-LABEL: @ffp1
+  // CHECK: atomicrmw fadd {{.*}} monotonic
+  return __c11_atomic_fetch_add(p, 1.0f, memory_order_relaxed);
+}
+
+float ffp2(float *p) {
+  // CHECK-LABEL: @ffp2
+  // CHECK: atomicrmw fsub {{.*}} monotonic
+  // CHECK: fsub
+  return __atomic_sub_fetch(p, 1.0, memory_order_relaxed);
+}
+
 int *fp2a(int **p) {
   // CHECK-LABEL: @fp2a
   // CHECK: store i32 4
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -4366,9 +4366,9 @@
   // For an arithmetic operation, the implied arithmetic must be well-formed.
   if (Form == Arithmetic) {
 // gcc does not enforce these rules for GNU atomics, but we do so for sanity.
-if (IsAddSub && !ValType->isIntegerType()
-&& !ValType->isP

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

In D71726#1801346 , @__simt__ wrote:

> In D71726#1792852 , @yaxunl wrote:
>
> > In D71726#1791904 , @jfb wrote:
> >
> > > This generally seems fine. Does it work on most backends? I want to make 
> > > sure it doesn't fail in backends :)
> >
> >
> > For x86_64, amdgcn, aarch64, armv7, mips64, it is translated to cmpxchg by 
> > AtomicExpandPass and backends did codegen successfully.
> >
> > For hexagon, riscv32, it is translated to call of `__atomic_fetch_add_4` 
> > for fadd float. This is concerning. Probably we need to add 
> > `__atomic_fetch_{add|sub}_{f16|f32|f64}` ?
>
>
> For systems that have load-link/store-conditional architectures, like ARM / 
> PPC / base RISC-V without extension, I would imagine that using a cmpxchg 
> loop is much worse than simply doing the floating-point add/sub in the middle 
> of the atomic mini-transaction. I'm sure that we want back-ends to be capable 
> of implementing this better than what this pass is doing, even when they 
> don't have "native" fp atomics.
>
> You listed amdgcn... what does this do on nvptx?


Targets can implement shouldExpandAtomicRMWInIR for the desired behavior, which 
NVPTX currently does not implement. Looking at AtomicExpandPass, it looks like 
either cmpxchg or LLSC expansions should work for the FP atomics already


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Olivier Giroux via Phabricator via cfe-commits]

__simt__ added a comment.

In D71726#1792852 , @yaxunl wrote:

> In D71726#1791904 , @jfb wrote:
>
> > This generally seems fine. Does it work on most backends? I want to make 
> > sure it doesn't fail in backends :)
>
>
> For x86_64, amdgcn, aarch64, armv7, mips64, it is translated to cmpxchg by 
> AtomicExpandPass and backends did codegen successfully.
>
> For hexagon, riscv32, it is translated to call of `__atomic_fetch_add_4` for 
> fadd float. This is concerning. Probably we need to add 
> `__atomic_fetch_{add|sub}_{f16|f32|f64}` ?


For systems that have load-link/store-conditional architectures, like ARM / PPC 
/ base RISC-V without extension, I would imagine that using a cmpxchg loop is 
much worse than simply doing the floating-point add/sub in the middle of the 
atomic mini-transaction. I'm sure that we want back-ends to be capable of 
implementing this better than what this pass is doing, even when they don't 
have "native" fp atomics.

You listed amdgcn... what does this do on nvptx?


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.



Comment at: clang/lib/CodeGen/CGAtomic.cpp:597-598
   case AtomicExpr::AO__atomic_add_fetch:
-PostOp = llvm::Instruction::Add;
+PostOp = E->getValueType()->isFloatingType() ? llvm::Instruction::FAdd
+ : llvm::Instruction::Add;
 LLVM_FALLTHROUGH;

Should this really be based on the type, or should the builtin name be 
different for FP?


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D71726#1791904 , @jfb wrote:

> This generally seems fine. Does it work on most backends? I want to make sure 
> it doesn't fail in backends :)


For x86_64, amdgcn, aarch64, armv7, mips64, it is translated to cmpxchg by 
AtomicExpandPass and backends did codegen successfully.

For hexagon, riscv32, it is translated to call of `__atomic_fetch_add_4` for 
fadd float. This is concerning. Probably we need to add 
`__atomic_fetch_{add|sub}_{f16|f32|f64}` ?


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[JF Bastien via Phabricator via cfe-commits]

jfb added subscribers: ldionne, EricWF, mclow.lists.
jfb added a comment.

This generally seems fine. Does it work on most backends? I want to make sure 
it doesn't fail in backends :)

Also, @ldionne / @EricWF / @mclow.lists do you need this in libc++ for 
floating-point atomic support?


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

https://reviews.llvm.org/D71726



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

[PATCH] D71726: Let clang atomic builtins fetch add/sub support floating point types

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl created this revision.
yaxunl added reviewers: rjmccall, b-sumner.
Herald added a subscriber: jfb.

Recently atomicrmw started to support fadd/fsub:

https://reviews.llvm.org/D53965

However clang atomic builtins fetch add/sub still does not support emitting 
atomicrmw fadd/fsub.

This patch adds that.


https://reviews.llvm.org/D71726

Files:
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGAtomic.cpp
  clang/lib/Sema/SemaChecking.cpp
  clang/test/CodeGen/atomic-ops.c
  clang/test/CodeGenOpenCL/atomic-ops.cl
  clang/test/Sema/atomic-ops.c
  clang/test/SemaOpenCL/atomic-ops.cl

Index: clang/test/SemaOpenCL/atomic-ops.cl
===
--- clang/test/SemaOpenCL/atomic-ops.cl
+++ clang/test/SemaOpenCL/atomic-ops.cl
@@ -70,7 +70,7 @@
 
   __opencl_atomic_fetch_add(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_add(p, 1, memory_order_seq_cst, memory_scope_work_group);
-  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer or pointer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
+  __opencl_atomic_fetch_add(d, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(i, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(p, 1, memory_order_seq_cst, memory_scope_work_group);
   __opencl_atomic_fetch_and(d, 1, memory_order_seq_cst, memory_scope_work_group); // expected-error {{address argument to atomic operation must be a pointer to atomic integer ('__generic atomic_float *' (aka '__generic _Atomic(float) *') invalid)}}
Index: clang/test/Sema/atomic-ops.c
===
--- clang/test/Sema/atomic-ops.c
+++ clang/test/Sema/atomic-ops.c
@@ -166,13 +166,13 @@
 
   __c11_atomic_fetch_add(i, 1, memory_order_seq_cst);
   __c11_atomic_fetch_add(p, 1, memory_order_seq_cst);
-  __c11_atomic_fetch_add(d, 1, memory_order_seq_cst); // expected-error {{must be a pointer to atomic integer or pointer}}
+  __c11_atomic_fetch_add(d, 1, memory_order_seq_cst);
 
-  __atomic_fetch_add(i, 3, memory_order_seq_cst); // expected-error {{pointer to integer or pointer}}
+  __atomic_fetch_add(i, 3, memory_order_seq_cst); // expected-error {{pointer to integer, pointer or floating point type}}
   __atomic_fetch_sub(I, 3, memory_order_seq_cst);
   __atomic_fetch_sub(P, 3, memory_order_seq_cst);
-  __atomic_fetch_sub(D, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
-  __atomic_fetch_sub(s1, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer or pointer}}
+  __atomic_fetch_sub(D, 3, memory_order_seq_cst);
+  __atomic_fetch_sub(s1, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer, pointer or floating point type}}
   __atomic_fetch_min(D, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer}}
   __atomic_fetch_max(P, 3, memory_order_seq_cst); // expected-error {{must be a pointer to integer}}
   __atomic_fetch_max(p, 3);   // expected-error {{too few arguments to function call, expected 3, have 2}}
Index: clang/test/CodeGenOpenCL/atomic-ops.cl
===
--- clang/test/CodeGenOpenCL/atomic-ops.cl
+++ clang/test/CodeGenOpenCL/atomic-ops.cl
@@ -185,6 +185,12 @@
   return __opencl_atomic_exchange(d, 2, memory_order_seq_cst, memory_scope_work_group);
 }
 
+float ff4(global atomic_float *d, float a) {
+  // CHECK-LABEL: @ff4
+  // CHECK: atomicrmw fadd float addrspace(1)* {{.*}} syncscope("workgroup-one-as") monotonic
+  return __opencl_atomic_fetch_add(d, a, memory_order_relaxed, memory_scope_work_group);
+}
+
 // CHECK-LABEL: @atomic_init_foo
 void atomic_init_foo()
 {
Index: clang/test/CodeGen/atomic-ops.c
===
--- clang/test/CodeGen/atomic-ops.c
+++ clang/test/CodeGen/atomic-ops.c
@@ -283,6 +283,19 @@
   return __c11_atomic_fetch_add(p, 1, memory_order_relaxed);
 }
 
+float ffp1(_Atomic(float) *p) {
+  // CHECK-LABEL: @ffp1
+  // CHECK: atomicrmw fadd {{.*}} monotonic
+  return __c11_atomic_fetch_add(p, 1.0f, memory_order_relaxed);
+}
+
+float ffp2(float *p) {
+  // CHECK-LABEL: @ffp2
+  // CHECK: atomicrmw fsub {{.*}} monotonic
+  // CHECK: fsub
+  return __atomic_sub_fetch(p, 1.0, memory_order_relaxed);
+}
+
 int *fp2a(int **p) {
   // CHECK-LABEL: @fp2a
   // CHECK: store i32 4
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -4755,9 +4755,9 @@
   // For an arithmetic operation, the implied arithmetic must be well-formed.
   if (Form == Arithmetic) {
 // gcc does not enforce these rules for GNU atom