[PATCH] D47154: Try to make builtin address space declarations not useless
arsenm closed this revision. arsenm added a comment. r338707 https://reviews.llvm.org/D47154 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
yaxunl accepted this revision. yaxunl added a comment. This revision is now accepted and ready to land. LGTM. Thanks. I missed the addr space casts you added to CodeGenFunction::EmitBuiltinExpr. With those casts it should work. For other downstream address space agnostic languages, e.g. (HCC), I guess they need to add similar hooks to use this feature. https://reviews.llvm.org/D47154 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
arsenm updated this revision to Diff 157885.
arsenm added a comment.
Remove old run line
https://reviews.llvm.org/D47154
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/BuiltinsAMDGPU.def
include/clang/Basic/TargetInfo.h
lib/AST/ASTContext.cpp
lib/Basic/Targets/AMDGPU.h
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaExpr.cpp
test/CodeGenCUDA/builtins-amdgcn.cu
test/CodeGenOpenCL/builtins-amdgcn.cl
test/CodeGenOpenCL/numbered-address-space.cl
test/SemaOpenCL/numbered-address-space.cl
Index: test/SemaOpenCL/numbered-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCL/numbered-address-space.cl
@@ -0,0 +1,31 @@
+// REQUIRES: amdgpu-registered-target
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-amd-amdhsa -verify -pedantic -fsyntax-only %s
+
+void test_numeric_as_to_generic_implicit_cast(__attribute__((address_space(3))) int *as3_ptr, float src) {
+ generic int* generic_ptr = as3_ptr; // FIXME: This should error
+}
+
+void test_numeric_as_to_generic_explicit_cast(__attribute__((address_space(3))) int *as3_ptr, float src) {
+ generic int* generic_ptr = (generic int*) as3_ptr; // Should maybe be valid?
+}
+
+void test_generic_to_numeric_as_implicit_cast() {
+ generic int* generic_ptr = 0;
+ __attribute__((address_space(3))) int *as3_ptr = generic_ptr; // expected-error{{initializing '__attribute__((address_space(3))) int *' with an expression of type '__generic int *' changes address space of pointer}}
+}
+
+void test_generic_to_numeric_as_explicit_cast() {
+ generic int* generic_ptr = 0;
+ __attribute__((address_space(3))) int *as3_ptr = (__attribute__((address_space(3))) int *)generic_ptr;
+}
+
+void test_generic_as_to_builtin_parameter_explicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
+ generic int* generic_ptr = as3_ptr; // FIXME: This should error
+ volatile float result = __builtin_amdgcn_ds_fmaxf((__attribute__((address_space(3))) float*) generic_ptr, src, 0, 0, false); // expected-error {{passing '__attribute__((address_space(3))) float *' to parameter of type '__local float *' changes address space of pointer}}
+}
+
+void test_generic_as_to_builtin_parameterimplicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
+ generic int* generic_ptr = as3_ptr;
+ volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-warning {{incompatible pointer types passing '__generic int *' to parameter of type '__local float *'}}
+}
+
Index: test/CodeGenOpenCL/numbered-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCL/numbered-address-space.cl
@@ -0,0 +1,34 @@
+// REQUIRES: amdgpu-registered-target
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-unknown-unknown -target-cpu tonga -S -emit-llvm -O0 -o - %s | FileCheck %s
+
+// Make sure using numbered address spaces doesn't trigger crashes when a
+// builtin has an address space parameter.
+
+// CHECK-LABEL: @test_numbered_as_to_generic(
+// CHECK: addrspacecast i32 addrspace(42)* %0 to i32*
+void test_numbered_as_to_generic(__attribute__((address_space(42))) int *arbitary_numbered_ptr) {
+ generic int* generic_ptr = arbitary_numbered_ptr;
+ *generic_ptr = 4;
+}
+
+// CHECK-LABEL: @test_numbered_as_to_builtin(
+// CHECK: addrspacecast i32 addrspace(42)* %0 to float addrspace(3)*
+void test_numbered_as_to_builtin(__attribute__((address_space(42))) int *arbitary_numbered_ptr, float src) {
+ volatile float result = __builtin_amdgcn_ds_fmaxf(arbitary_numbered_ptr, src, 0, 0, false);
+}
+
+// CHECK-LABEL: @test_generic_as_to_builtin_parameter_explicit_cast(
+// CHECK: addrspacecast i32 addrspace(3)* %0 to i32*
+void test_generic_as_to_builtin_parameter_explicit_cast(__local int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+ volatile float result = __builtin_amdgcn_ds_fmaxf((__local float*) generic_ptr, src, 0, 0, false);
+}
+
+// CHECK-LABEL: @test_generic_as_to_builtin_parameter_implicit_cast(
+// CHECK: addrspacecast i32* %2 to float addrspace(3)*
+void test_generic_as_to_builtin_parameter_implicit_cast(__local int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+
+ volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false);
+}
+
Index: test/CodeGenOpenCL/builtins-amdgcn.cl
===
--- test/CodeGenOpenCL/builtins-amdgcn.cl
+++ test/CodeGenOpenCL/builtins-amdgcn.cl
@@ -1,6 +1,5 @@
// REQUIRES: amdgpu-registered-target
-// RUN: %clang_cc1 -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
-// RUN: %clang_cc1 -triple amdgcn-unknown-unknown-opencl -S -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck -enable-var-scope %s
#pragma OPENCL EXTENSION cl_khr_fp64 : enable
@@ -20,19
[PATCH] D47154: Try to make builtin address space declarations not useless
arsenm added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:1157
+ /// language address space.
+ virtual LangAS getCUDABuiltinAddressSpace(unsigned AS) const {
+return getLangASFromTargetAS(AS);
yaxunl wrote:
> I think this function is not needed.
>
> Although CUDA/HIP uses address spaces in codegen, but it does not use named
> address spaces in sema and AST. Named address space is not in the AST types
> of CUDA/HIP, therefore there is no point of mapping target address space back
> to language address space for CUDA/HIP.
>
> CUDA/HIP should be just like other address-space-agnostic language and always
> use getLangASFromTargetAS(AS).
This is necessary to insert the correct addrspacecast, otherwise the builtins
CUDA test asserts. getLangASFromTargetAS returns the user specified addrspace
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
yaxunl added a comment. In https://reviews.llvm.org/D47154#1108813, @tra wrote: > CUDA does not expose explicit AS on clang size. All pointers are treated as > generic and we infer specific address space only in LLVM. > `__nvvm_atom_*_[sg]_*` builtins should probably be removed as they are > indeed useless without pointers with explicit AS and NVCC itself does not > have such builtins either. Instead, we should convert the generic AS builtin > to address-space specific instruction somewhere in LLVM. > > Using `attribute((address_space())` should probably produce an error during > CUDA compilation. Sometimes we need to call functions defined in our device library which is written in OpenCL. Some function have pointer arguments in non-zero address space. To declare these functions in CUDA/HIP we need to use `__attribute__((address_space()))`. We use C-style cast to cast pointers in CUDA/HIP to a non-zero address space and pass them to the functions. I think `__attribute__((address_space()))` is still needed for this situation. https://reviews.llvm.org/D47154 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
yaxunl added inline comments. Comment at: test/CodeGenOpenCL/builtins-amdgcn.cl:3 +// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck -enable-var-scope %s +// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-unknown-unknown-opencl -S -emit-llvm -o - %s | FileCheck -enable-var-scope %s Please remove this line since we no longer use opencl in triple. https://reviews.llvm.org/D47154 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
yaxunl added inline comments.
Comment at: include/clang/Basic/TargetInfo.h:1157
+ /// language address space.
+ virtual LangAS getCUDABuiltinAddressSpace(unsigned AS) const {
+return getLangASFromTargetAS(AS);
I think this function is not needed.
Although CUDA/HIP uses address spaces in codegen, but it does not use named
address spaces in sema and AST. Named address space is not in the AST types of
CUDA/HIP, therefore there is no point of mapping target address space back to
language address space for CUDA/HIP.
CUDA/HIP should be just like other address-space-agnostic language and always
use getLangASFromTargetAS(AS).
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
yaxunl requested changes to this revision.
yaxunl added inline comments.
This revision now requires changes to proceed.
Comment at: lib/Basic/Targets/AMDGPU.h:398
+
+ LangAS getCUDABuiltinAddressSpace(unsigned AS) const override {
+return LangAS::Default;
I am wondering how this would work for CUDA/HIP.
Let's say a builtin is supposed to return a pointer to addrspace 4.
Now in HIP this builtin is returning a pointer to addrspace 0.
How would that work?
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
Anastasia accepted this revision.
Anastasia added a comment.
This revision is now accepted and ready to land.
LGTM from OpenCL side! Thanks!
Comment at: test/SemaOpenCL/numbered-address-space.cl:9
+void
test_numeric_as_to_generic_explicit_cast(__attribute__((address_space(3))) int
*as3_ptr, float src) {
+ generic int* generic_ptr = (generic int*) as3_ptr; // Should maybe be valid?
+}
Ideally this is not governed by any specification. Generic compiler support for
such `addrspacecast` is not possible but vendor can implement custom support. I
think we can leave it as is until we have better idea how this should be
supported.
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
arsenm added a comment. ping https://reviews.llvm.org/D47154 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
arsenm added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:3500
+if (auto *PtrTy = dyn_cast(PTy)) {
+ if (PtrTy->getAddressSpace() !=
+ ArgValue->getType()->getPointerAddressSpace()) {
Anastasia wrote:
> arsenm wrote:
> > Anastasia wrote:
> > > arsenm wrote:
> > > > Anastasia wrote:
> > > > > Would this be correct for OpenCL? Should we use
> > > > > `isAddressSpaceSupersetOf` helper instead? Would it also sort the
> > > > > issue with constant AS (at least for OpenCL)?
> > > > The issue I mentioned for the other builtin is that it modifies the
> > > > memory, and doesn't have to do with the casting.
> > > >
> > > > At this point the AddrSpaceCast has to be emitted. The checking if the
> > > > cast is legal I guess would be in the SemaExpr part. I know at one
> > > > point I was trying to use isAddressSpaceSupersetOf in
> > > > rewriteBuiltinFunctionDecl, but there was some problem with that. I
> > > > think it didn't make sense with the magic where the builtin without an
> > > > address space is supposed to accept any address space or something
> > > > along those lines.
> > > Yes, I think Sema has to check it before indeed. I am not sure it works
> > > right with OpenCL rules though for the Builtin functions. Would it make
> > > sense to add a negative test for this then?
> > I'm not sure what this test would look like. Do you mean a test that
> > erroneously is accepted now?
> Ok, so at this point you are trying to change generation of `bitcast` to
> `addrspacecast` which makes sense to me. Do we still need a `bitcast` though?
>
> I think `addrspacecast` can be used to convert type and address space too:
> The ‘addrspacecast‘ instruction converts ptrval from pty in address space n
> to type pty2 in address space m.
>
> It would be nice to add proper Sema checking for `Builtins` for address space
> of pointers in OpenCL mode, but this might be more work.
I think the canonical form is to use the bitcast for the type pointer
conversion, and then separate the addrspacecast. I think instcombine splits
these apart
Comment at: test/CodeGenOpenCL/numbered-address-space.cl:36
+#if 0
+// XXX: Should this compile?
+void
test_generic_as_to_builtin_parameter_explicit_cast_numeric(__attribute__((address_space(3)))
int *local_ptr, float src) {
Anastasia wrote:
> arsenm wrote:
> > Anastasia wrote:
> > > `__attribute__((address_space(N)))` is not an OpenCL feature and I think
> > > it's not specified in C either? But I think generally non matching
> > > address spaces don't compile in Clang. So it might be useful to disallow
> > > this?
> > I'm pretty sure it's a C extension. The way things seem to work now is
> > address spaces are accepted anywhere and everywhere.
> Yes, the line below should give an error for OpenCL?
> generic int* generic_ptr = local_ptr;
This does not error. The wording of the spec seems to leave some interpretation
for other address spaces. It whitelists the valid address spaces for implicit
casts, and blacklists constant for implicit or explicit casts. My reading
between the lines is that an explicit cast would be OK. I think this is a
separate fix since this is independent from the builtins
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
arsenm updated this revision to Diff 154561.
arsenm added a comment.
Add sema test for numbered address spaces
https://reviews.llvm.org/D47154
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/BuiltinsAMDGPU.def
include/clang/Basic/TargetInfo.h
lib/AST/ASTContext.cpp
lib/Basic/Targets/AMDGPU.h
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaExpr.cpp
test/CodeGenCUDA/builtins-amdgcn.cu
test/CodeGenOpenCL/builtins-amdgcn.cl
test/CodeGenOpenCL/numbered-address-space.cl
test/SemaOpenCL/numbered-address-space.cl
Index: test/SemaOpenCL/numbered-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCL/numbered-address-space.cl
@@ -0,0 +1,31 @@
+// REQUIRES: amdgpu-registered-target
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-amd-amdhsa -verify -pedantic -fsyntax-only %s
+
+void test_numeric_as_to_generic_implicit_cast(__attribute__((address_space(3))) int *as3_ptr, float src) {
+ generic int* generic_ptr = as3_ptr; // FIXME: This should error
+}
+
+void test_numeric_as_to_generic_explicit_cast(__attribute__((address_space(3))) int *as3_ptr, float src) {
+ generic int* generic_ptr = (generic int*) as3_ptr; // Should maybe be valid?
+}
+
+void test_generic_to_numeric_as_implicit_cast() {
+ generic int* generic_ptr = 0;
+ __attribute__((address_space(3))) int *as3_ptr = generic_ptr; // expected-error{{initializing '__attribute__((address_space(3))) int *' with an expression of type '__generic int *' changes address space of pointer}}
+}
+
+void test_generic_to_numeric_as_explicit_cast() {
+ generic int* generic_ptr = 0;
+ __attribute__((address_space(3))) int *as3_ptr = (__attribute__((address_space(3))) int *)generic_ptr;
+}
+
+void test_generic_as_to_builtin_parameter_explicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
+ generic int* generic_ptr = as3_ptr; // FIXME: This should error
+ volatile float result = __builtin_amdgcn_ds_fmaxf((__attribute__((address_space(3))) float*) generic_ptr, src, 0, 0, false); // expected-error {{passing '__attribute__((address_space(3))) float *' to parameter of type '__local float *' changes address space of pointer}}
+}
+
+void test_generic_as_to_builtin_parameterimplicit_cast_numeric(__attribute__((address_space(3))) int *as3_ptr, float src) {
+ generic int* generic_ptr = as3_ptr;
+ volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false); // expected-warning {{incompatible pointer types passing '__generic int *' to parameter of type '__local float *'}}
+}
+
Index: test/CodeGenOpenCL/numbered-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCL/numbered-address-space.cl
@@ -0,0 +1,34 @@
+// REQUIRES: amdgpu-registered-target
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-unknown-unknown -target-cpu tonga -S -emit-llvm -O0 -o - %s | FileCheck %s
+
+// Make sure using numbered address spaces doesn't trigger crashes when a
+// builtin has an address space parameter.
+
+// CHECK-LABEL: @test_numbered_as_to_generic(
+// CHECK: addrspacecast i32 addrspace(42)* %0 to i32*
+void test_numbered_as_to_generic(__attribute__((address_space(42))) int *arbitary_numbered_ptr) {
+ generic int* generic_ptr = arbitary_numbered_ptr;
+ *generic_ptr = 4;
+}
+
+// CHECK-LABEL: @test_numbered_as_to_builtin(
+// CHECK: addrspacecast i32 addrspace(42)* %0 to float addrspace(3)*
+void test_numbered_as_to_builtin(__attribute__((address_space(42))) int *arbitary_numbered_ptr, float src) {
+ volatile float result = __builtin_amdgcn_ds_fmaxf(arbitary_numbered_ptr, src, 0, 0, false);
+}
+
+// CHECK-LABEL: @test_generic_as_to_builtin_parameter_explicit_cast(
+// CHECK: addrspacecast i32 addrspace(3)* %0 to i32*
+void test_generic_as_to_builtin_parameter_explicit_cast(__local int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+ volatile float result = __builtin_amdgcn_ds_fmaxf((__local float*) generic_ptr, src, 0, 0, false);
+}
+
+// CHECK-LABEL: @test_generic_as_to_builtin_parameter_implicit_cast(
+// CHECK: addrspacecast i32* %2 to float addrspace(3)*
+void test_generic_as_to_builtin_parameter_implicit_cast(__local int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+
+ volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false);
+}
+
Index: test/CodeGenOpenCL/builtins-amdgcn.cl
===
--- test/CodeGenOpenCL/builtins-amdgcn.cl
+++ test/CodeGenOpenCL/builtins-amdgcn.cl
@@ -1,6 +1,6 @@
// REQUIRES: amdgpu-registered-target
-// RUN: %clang_cc1 -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
-// RUN: %clang_cc1 -triple amdgcn-unknown-unknown-opencl -S -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck -enable-var-scope %s
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple
[PATCH] D47154: Try to make builtin address space declarations not useless
Anastasia added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:3500
+if (auto *PtrTy = dyn_cast(PTy)) {
+ if (PtrTy->getAddressSpace() !=
+ ArgValue->getType()->getPointerAddressSpace()) {
arsenm wrote:
> Anastasia wrote:
> > arsenm wrote:
> > > Anastasia wrote:
> > > > Would this be correct for OpenCL? Should we use
> > > > `isAddressSpaceSupersetOf` helper instead? Would it also sort the issue
> > > > with constant AS (at least for OpenCL)?
> > > The issue I mentioned for the other builtin is that it modifies the
> > > memory, and doesn't have to do with the casting.
> > >
> > > At this point the AddrSpaceCast has to be emitted. The checking if the
> > > cast is legal I guess would be in the SemaExpr part. I know at one point
> > > I was trying to use isAddressSpaceSupersetOf in
> > > rewriteBuiltinFunctionDecl, but there was some problem with that. I think
> > > it didn't make sense with the magic where the builtin without an address
> > > space is supposed to accept any address space or something along those
> > > lines.
> > Yes, I think Sema has to check it before indeed. I am not sure it works
> > right with OpenCL rules though for the Builtin functions. Would it make
> > sense to add a negative test for this then?
> I'm not sure what this test would look like. Do you mean a test that
> erroneously is accepted now?
Ok, so at this point you are trying to change generation of `bitcast` to
`addrspacecast` which makes sense to me. Do we still need a `bitcast` though?
I think `addrspacecast` can be used to convert type and address space too:
The ‘addrspacecast‘ instruction converts ptrval from pty in address space n
to type pty2 in address space m.
It would be nice to add proper Sema checking for `Builtins` for address space
of pointers in OpenCL mode, but this might be more work.
Comment at: test/CodeGenOpenCL/numbered-address-space.cl:36
+#if 0
+// XXX: Should this compile?
+void
test_generic_as_to_builtin_parameter_explicit_cast_numeric(__attribute__((address_space(3)))
int *local_ptr, float src) {
arsenm wrote:
> Anastasia wrote:
> > `__attribute__((address_space(N)))` is not an OpenCL feature and I think
> > it's not specified in C either? But I think generally non matching address
> > spaces don't compile in Clang. So it might be useful to disallow this?
> I'm pretty sure it's a C extension. The way things seem to work now is
> address spaces are accepted anywhere and everywhere.
Yes, the line below should give an error for OpenCL?
generic int* generic_ptr = local_ptr;
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
arsenm added inline comments.
Comment at: lib/CodeGen/CGBuiltin.cpp:3500
+if (auto *PtrTy = dyn_cast(PTy)) {
+ if (PtrTy->getAddressSpace() !=
+ ArgValue->getType()->getPointerAddressSpace()) {
Anastasia wrote:
> arsenm wrote:
> > Anastasia wrote:
> > > Would this be correct for OpenCL? Should we use
> > > `isAddressSpaceSupersetOf` helper instead? Would it also sort the issue
> > > with constant AS (at least for OpenCL)?
> > The issue I mentioned for the other builtin is that it modifies the memory,
> > and doesn't have to do with the casting.
> >
> > At this point the AddrSpaceCast has to be emitted. The checking if the cast
> > is legal I guess would be in the SemaExpr part. I know at one point I was
> > trying to use isAddressSpaceSupersetOf in rewriteBuiltinFunctionDecl, but
> > there was some problem with that. I think it didn't make sense with the
> > magic where the builtin without an address space is supposed to accept any
> > address space or something along those lines.
> Yes, I think Sema has to check it before indeed. I am not sure it works right
> with OpenCL rules though for the Builtin functions. Would it make sense to
> add a negative test for this then?
I'm not sure what this test would look like. Do you mean a test that
erroneously is accepted now?
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
Anastasia added inline comments.
Comment at: include/clang/Basic/BuiltinsAMDGPU.def:49
+
+// FIXME: Need to disallow constant address space.
BUILTIN(__builtin_amdgcn_div_scale, "dddbb*", "n")
arsenm wrote:
> Anastasia wrote:
> > Do you plan to provide the support for it later? Or if else perhaps we
> > should elaborate more what's to be done.
> I'm not sure. I don't know how to best enforce this
The only way I guess if we list overloads with each address space explicitly
(apart from `constant`)? Or may be with the use of `generic` AS, although that
will only work for CL2.0.
Comment at: lib/AST/ASTContext.cpp:9093
unsigned AddrSpace = strtoul(Str, , 10);
- if (End != Str && AddrSpace != 0) {
-Type = Context.getAddrSpaceQualType(Type,
-getLangASFromTargetAS(AddrSpace));
+ if (End != Str) {
+// Note AddrSpace == 0 is not the same as an unspecified address space.
arsenm wrote:
> Anastasia wrote:
> > Could we check against LangAS::Default instead of removing this completely.
> I don't think that really make sense, since that would be leaving this the
> same. I don't really need it for this patch, but I fundamentally think
> specifying address space 0 is different from an unspecified address space.
> According to the description for builtins, if no address space is specified
> than any address space will be accepted. This is different from a builtin
> requiring address space 0
I thought `Default` AS was meant to be for the case no AS is specified but I
guess it doesn't work the same in Builtins specification syntax.
Comment at: lib/CodeGen/CGBuiltin.cpp:3500
+if (auto *PtrTy = dyn_cast(PTy)) {
+ if (PtrTy->getAddressSpace() !=
+ ArgValue->getType()->getPointerAddressSpace()) {
arsenm wrote:
> Anastasia wrote:
> > Would this be correct for OpenCL? Should we use `isAddressSpaceSupersetOf`
> > helper instead? Would it also sort the issue with constant AS (at least for
> > OpenCL)?
> The issue I mentioned for the other builtin is that it modifies the memory,
> and doesn't have to do with the casting.
>
> At this point the AddrSpaceCast has to be emitted. The checking if the cast
> is legal I guess would be in the SemaExpr part. I know at one point I was
> trying to use isAddressSpaceSupersetOf in rewriteBuiltinFunctionDecl, but
> there was some problem with that. I think it didn't make sense with the magic
> where the builtin without an address space is supposed to accept any address
> space or something along those lines.
Yes, I think Sema has to check it before indeed. I am not sure it works right
with OpenCL rules though for the Builtin functions. Would it make sense to add
a negative test for this then?
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
arsenm updated this revision to Diff 150179.
arsenm added a comment.
Rebase and add comment
https://reviews.llvm.org/D47154
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/BuiltinsAMDGPU.def
include/clang/Basic/TargetInfo.h
lib/AST/ASTContext.cpp
lib/Basic/Targets/AMDGPU.h
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaExpr.cpp
test/CodeGenCUDA/builtins-amdgcn.cu
test/CodeGenOpenCL/builtins-amdgcn.cl
test/CodeGenOpenCL/numbered-address-space.cl
Index: test/CodeGenOpenCL/numbered-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCL/numbered-address-space.cl
@@ -0,0 +1,47 @@
+// REQUIRES: amdgpu-registered-target
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-unknown-unknown -target-cpu tonga -S -emit-llvm -O0 -o - %s | FileCheck %s
+
+// Make sure using numbered address spaces doesn't trigger crashes when a
+// builtin has an address space parameter.
+
+// CHECK-LABEL: @test_numbered_as_to_generic(
+// CHECK: addrspacecast i32 addrspace(42)* %0 to i32*
+void test_numbered_as_to_generic(__attribute__((address_space(42))) int *arbitary_numbered_ptr) {
+ generic int* generic_ptr = arbitary_numbered_ptr;
+ *generic_ptr = 4;
+}
+
+// CHECK-LABEL: @test_numbered_as_to_builtin(
+// CHECK: addrspacecast i32 addrspace(42)* %0 to float addrspace(3)*
+void test_numbered_as_to_builtin(__attribute__((address_space(42))) int *arbitary_numbered_ptr, float src) {
+ volatile float result = __builtin_amdgcn_ds_fmaxf(arbitary_numbered_ptr, src, 0, 0, false);
+}
+
+// CHECK-LABEL: @test_generic_as_to_builtin_parameter_explicit_cast(
+// CHECK: addrspacecast i32 addrspace(3)* %0 to i32*
+void test_generic_as_to_builtin_parameter_explicit_cast(__local int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+ volatile float result = __builtin_amdgcn_ds_fmaxf((__local float*) generic_ptr, src, 0, 0, false);
+}
+
+// CHECK-LABEL: @test_generic_as_to_builtin_parameter_implicit_cast(
+// CHECK: addrspacecast i32* %2 to float addrspace(3)*
+void test_generic_as_to_builtin_parameter_implicit_cast(__local int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+
+ volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false);
+}
+
+#if 0
+// XXX: Should this compile?
+void test_generic_as_to_builtin_parameter_explicit_cast_numeric(__attribute__((address_space(3))) int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+ volatile float result = __builtin_amdgcn_ds_fmaxf((__attribute__((address_space(3))) float*) generic_ptr, src, 0, 0, false);
+}
+
+// XXX: Should this compile?
+void test_generic_as_to_builtin_parameterimplicit_cast_numeric(__attribute__((address_space(3))) int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+ volatile float result = __builtin_amdgcn_ds_fmaxf(generic_ptr, src, 0, 0, false);
+}
+#endif
Index: test/CodeGenOpenCL/builtins-amdgcn.cl
===
--- test/CodeGenOpenCL/builtins-amdgcn.cl
+++ test/CodeGenOpenCL/builtins-amdgcn.cl
@@ -1,6 +1,6 @@
// REQUIRES: amdgpu-registered-target
-// RUN: %clang_cc1 -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
-// RUN: %clang_cc1 -triple amdgcn-unknown-unknown-opencl -S -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck -enable-var-scope %s
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-unknown-unknown-opencl -S -emit-llvm -o - %s | FileCheck -enable-var-scope %s
#pragma OPENCL EXTENSION cl_khr_fp64 : enable
@@ -20,19 +20,42 @@
*flagout = flag;
}
-// CHECK-LABEL: @test_div_scale_f32
+// CHECK-LABEL: @test_div_scale_f32(
// CHECK: call { float, i1 } @llvm.amdgcn.div.scale.f32(float %a, float %b, i1 true)
// CHECK-DAG: [[FLAG:%.+]] = extractvalue { float, i1 } %{{.+}}, 1
// CHECK-DAG: [[VAL:%.+]] = extractvalue { float, i1 } %{{.+}}, 0
-// CHECK: [[FLAGEXT:%.+]] = zext i1 [[FLAG]] to i32
-// CHECK: store i32 [[FLAGEXT]]
-void test_div_scale_f32(global float* out, global int* flagout, float a, float b)
+// CHECK: [[FLAGEXT:%.+]] = zext i1 [[FLAG]] to i8
+// CHECK: store i8 [[FLAGEXT]]
+void test_div_scale_f32(global float* out, global bool* flagout, float a, float b)
{
bool flag;
*out = __builtin_amdgcn_div_scalef(a, b, true, );
*flagout = flag;
}
+// CHECK-LABEL: @test_div_scale_f32_global_ptr(
+// CHECK: call { float, i1 } @llvm.amdgcn.div.scale.f32(float %a, float %b, i1 true)
+// CHECK-DAG: [[FLAG:%.+]] = extractvalue { float, i1 } %{{.+}}, 1
+// CHECK-DAG: [[VAL:%.+]] = extractvalue { float, i1 } %{{.+}}, 0
+// CHECK: [[FLAGEXT:%.+]] = zext i1 [[FLAG]] to i8
+// CHECK: store i8 [[FLAGEXT]]
+void test_div_scale_f32_global_ptr(global float* out, global int* flagout, float a, float b, global bool* flag)
+{
+ *out = __builtin_amdgcn_div_scalef(a, b, true, flag);
+}
+
+// CHECK-LABEL:
[PATCH] D47154: Try to make builtin address space declarations not useless
arsenm added inline comments.
Comment at: include/clang/Basic/BuiltinsAMDGPU.def:49
+
+// FIXME: Need to disallow constant address space.
BUILTIN(__builtin_amdgcn_div_scale, "dddbb*", "n")
Anastasia wrote:
> Do you plan to provide the support for it later? Or if else perhaps we should
> elaborate more what's to be done.
I'm not sure. I don't know how to best enforce this
Comment at: lib/AST/ASTContext.cpp:9093
unsigned AddrSpace = strtoul(Str, , 10);
- if (End != Str && AddrSpace != 0) {
-Type = Context.getAddrSpaceQualType(Type,
-getLangASFromTargetAS(AddrSpace));
+ if (End != Str) {
+// Note AddrSpace == 0 is not the same as an unspecified address space.
Anastasia wrote:
> Could we check against LangAS::Default instead of removing this completely.
I don't think that really make sense, since that would be leaving this the
same. I don't really need it for this patch, but I fundamentally think
specifying address space 0 is different from an unspecified address space.
According to the description for builtins, if no address space is specified
than any address space will be accepted. This is different from a builtin
requiring address space 0
Comment at: lib/CodeGen/CGBuiltin.cpp:3500
+if (auto *PtrTy = dyn_cast(PTy)) {
+ if (PtrTy->getAddressSpace() !=
+ ArgValue->getType()->getPointerAddressSpace()) {
Anastasia wrote:
> Would this be correct for OpenCL? Should we use `isAddressSpaceSupersetOf`
> helper instead? Would it also sort the issue with constant AS (at least for
> OpenCL)?
The issue I mentioned for the other builtin is that it modifies the memory, and
doesn't have to do with the casting.
At this point the AddrSpaceCast has to be emitted. The checking if the cast is
legal I guess would be in the SemaExpr part. I know at one point I was trying
to use isAddressSpaceSupersetOf in rewriteBuiltinFunctionDecl, but there was
some problem with that. I think it didn't make sense with the magic where the
builtin without an address space is supposed to accept any address space or
something along those lines.
Comment at: test/CodeGenOpenCL/numbered-address-space.cl:36
+#if 0
+// XXX: Should this compile?
+void
test_generic_as_to_builtin_parameter_explicit_cast_numeric(__attribute__((address_space(3)))
int *local_ptr, float src) {
Anastasia wrote:
> `__attribute__((address_space(N)))` is not an OpenCL feature and I think it's
> not specified in C either? But I think generally non matching address spaces
> don't compile in Clang. So it might be useful to disallow this?
I'm pretty sure it's a C extension. The way things seem to work now is address
spaces are accepted anywhere and everywhere.
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
tra added a comment. CUDA does not expose explicit AS on clang size. All pointers are treated as generic and we infer specific address space only in LLVM. `__nvvm_atom_*_[sg]_*` builtins should probably be removed as they are indeed useless without pointers with explicit AS and NVCC itself does not have such builtins either. Instead, we should convert the generic AS builtin to address-space specific instruction somewhere in LLVM. Using `attribute((address_space())` should probably produce an error during CUDA compilation. https://reviews.llvm.org/D47154 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
Anastasia added inline comments.
Comment at: include/clang/Basic/BuiltinsAMDGPU.def:49
+
+// FIXME: Need to disallow constant address space.
BUILTIN(__builtin_amdgcn_div_scale, "dddbb*", "n")
Do you plan to provide the support for it later? Or if else perhaps we should
elaborate more what's to be done.
Comment at: include/clang/Basic/TargetInfo.h:1009
+ virtual LangAS getOpenCLBuiltinAddressSpace(unsigned AS) const {
+return getLangASFromTargetAS(AS);
Can you add a comment please to explain what the function is for?
Comment at: lib/AST/ASTContext.cpp:9093
unsigned AddrSpace = strtoul(Str, , 10);
- if (End != Str && AddrSpace != 0) {
-Type = Context.getAddrSpaceQualType(Type,
-getLangASFromTargetAS(AddrSpace));
+ if (End != Str) {
+// Note AddrSpace == 0 is not the same as an unspecified address space.
Could we check against LangAS::Default instead of removing this completely.
Comment at: lib/CodeGen/CGBuiltin.cpp:3500
+if (auto *PtrTy = dyn_cast(PTy)) {
+ if (PtrTy->getAddressSpace() !=
+ ArgValue->getType()->getPointerAddressSpace()) {
Would this be correct for OpenCL? Should we use `isAddressSpaceSupersetOf`
helper instead? Would it also sort the issue with constant AS (at least for
OpenCL)?
Comment at: test/CodeGenOpenCL/numbered-address-space.cl:36
+#if 0
+// XXX: Should this compile?
+void
test_generic_as_to_builtin_parameter_explicit_cast_numeric(__attribute__((address_space(3)))
int *local_ptr, float src) {
`__attribute__((address_space(N)))` is not an OpenCL feature and I think it's
not specified in C either? But I think generally non matching address spaces
don't compile in Clang. So it might be useful to disallow this?
https://reviews.llvm.org/D47154
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D47154: Try to make builtin address space declarations not useless
arsenm created this revision.
arsenm added reviewers: Anastasia, yaxunl, rjmccall.
Herald added subscribers: tpr, nhaehnle, wdng.
The way address space declarations for builtins currently work
is nearly useless. The code assumes the address spaces used for
builtins is a confusingly named "target address space" from user
code using __attribute__((address_space(N))) that matches
the builtin declaration. There's no way to use this to declare
a builtin that returns a language specific address space.
The terminology used is highly cofusing since it has nothing
to do with the the address space selected by the target to use
for a language address space.
This feature is essentially unused as-is. AMDGPU and NVPTX
are the only in-tree targets attempting to use this. The AMDGPU
builtins certainly do not behave as intended (i.e. all of the
builtins returning pointers can never compile because the numbered
address space never matches the expected named address space).
The NVPTX builtins are missing tests for some, and the others
seem to rely on an implicit addrspacecast.
Change the used address space for builtins based on a target
hook to allow using a language address space for a builtin.
This allows the same builtin declaration to be used for multiple
languages with similarly purposed address spaces (e.g. the same
AMDGPU builtin can be used in OpenCL and CUDA even though the
constant address spaces are arbitarily different).
This breaks the possibility of using arbitrary numbered
address spaces alongside the named address spaces for builtins.
If this is an issue we probably need to introduce another builtin
declaration character to distinguish language address spaces from
so-called "target address spaces".
https://reviews.llvm.org/D47154
Files:
include/clang/AST/ASTContext.h
include/clang/Basic/BuiltinsAMDGPU.def
include/clang/Basic/TargetInfo.h
lib/AST/ASTContext.cpp
lib/Basic/Targets/AMDGPU.h
lib/CodeGen/CGBuiltin.cpp
lib/Sema/SemaExpr.cpp
test/CodeGenCUDA/builtins-amdgcn.cu
test/CodeGenOpenCL/builtins-amdgcn-vi.cl
test/CodeGenOpenCL/builtins-amdgcn.cl
test/CodeGenOpenCL/numbered-address-space.cl
Index: test/CodeGenOpenCL/numbered-address-space.cl
===
--- /dev/null
+++ test/CodeGenOpenCL/numbered-address-space.cl
@@ -0,0 +1,47 @@
+// REQUIRES: amdgpu-registered-target
+// RUN: %clang_cc1 -cl-std=CL2.0 -triple amdgcn-unknown-unknown -target-cpu tonga -S -emit-llvm -O0 -o - %s | FileCheck %s
+
+// Make sure using numbered address spaces doesn't trigger crashes when a
+// builtin has an address space parameter.
+
+// CHECK-LABEL: @test_numbered_as_to_generic(
+// CHECK: addrspacecast i32 addrspace(42)* %0 to i32*
+void test_numbered_as_to_generic(__attribute__((address_space(42))) int *arbitary_numbered_ptr) {
+ generic int* generic_ptr = arbitary_numbered_ptr;
+ *generic_ptr = 4;
+}
+
+// CHECK-LABEL: @test_numbered_as_to_builtin(
+// CHECK: addrspacecast i32 addrspace(42)* %0 to float addrspace(3)*
+void test_numbered_as_to_builtin(__attribute__((address_space(42))) int *arbitary_numbered_ptr, float src) {
+ volatile float result = __builtin_amdgcn_ds_fmax(arbitary_numbered_ptr, src, 0, 0, false);
+}
+
+// CHECK-LABEL: @test_generic_as_to_builtin_parameter_explicit_cast(
+// CHECK: addrspacecast i32 addrspace(3)* %0 to i32*
+void test_generic_as_to_builtin_parameter_explicit_cast(__local int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+ volatile float result = __builtin_amdgcn_ds_fmax((__local float*) generic_ptr, src, 0, 0, false);
+}
+
+// CHECK-LABEL: @test_generic_as_to_builtin_parameter_implicit_cast(
+// CHECK: addrspacecast i32* %2 to float addrspace(3)*
+void test_generic_as_to_builtin_parameter_implicit_cast(__local int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+
+ volatile float result = __builtin_amdgcn_ds_fmax(generic_ptr, src, 0, 0, false);
+}
+
+#if 0
+// XXX: Should this compile?
+void test_generic_as_to_builtin_parameter_explicit_cast_numeric(__attribute__((address_space(3))) int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+ volatile float result = __builtin_amdgcn_ds_fmax((__attribute__((address_space(3))) float*) generic_ptr, src, 0, 0, false);
+}
+
+// XXX: Should this compile?
+void test_generic_as_to_builtin_parameterimplicit_cast_numeric(__attribute__((address_space(3))) int *local_ptr, float src) {
+ generic int* generic_ptr = local_ptr;
+ volatile float result = __builtin_amdgcn_ds_fmax(generic_ptr, src, 0, 0, false);
+}
+#endif
Index: test/CodeGenOpenCL/builtins-amdgcn.cl
===
--- test/CodeGenOpenCL/builtins-amdgcn.cl
+++ test/CodeGenOpenCL/builtins-amdgcn.cl
@@ -1,6 +1,6 @@
// REQUIRES: amdgpu-registered-target
-// RUN: %clang_cc1 -triple amdgcn-unknown-unknown -S -emit-llvm -o - %s | FileCheck %s
-// RUN: %clang_cc1 -triple
