[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL299965: [OpenCL] Map default address space to alloca address 
space (authored by yaxunl).

Changed prior to commit:
  https://reviews.llvm.org/D31404?vs=94399=94855#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D31404

Files:
  cfe/trunk/include/clang/AST/ASTContext.h
  cfe/trunk/include/clang/AST/Type.h
  cfe/trunk/include/clang/Basic/AddressSpaces.h
  cfe/trunk/lib/AST/ASTContext.cpp
  cfe/trunk/lib/AST/ExprClassification.cpp
  cfe/trunk/lib/AST/TypePrinter.cpp
  cfe/trunk/lib/Basic/Targets.cpp
  cfe/trunk/lib/Sema/SemaExprCXX.cpp
  cfe/trunk/lib/Sema/SemaOverload.cpp
  cfe/trunk/lib/Sema/SemaType.cpp
  cfe/trunk/test/CodeGen/address-space.c
  cfe/trunk/test/CodeGen/default-address-space.c
  cfe/trunk/test/CodeGenOpenCL/address-spaces.cl
  cfe/trunk/test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
  cfe/trunk/test/CodeGenOpenCL/vla.cl
  cfe/trunk/test/Sema/address_spaces.c
  cfe/trunk/test/Sema/invalid-assignment-constant-address-space.c
  cfe/trunk/test/SemaOpenCL/invalid-assignment-constant-address-space.cl

Index: cfe/trunk/include/clang/AST/Type.h
===
--- cfe/trunk/include/clang/AST/Type.h
+++ cfe/trunk/include/clang/AST/Type.h
@@ -333,6 +333,20 @@
 
   bool hasAddressSpace() const { return Mask & AddressSpaceMask; }
   unsigned getAddressSpace() const { return Mask >> AddressSpaceShift; }
+  /// Get the address space attribute value to be printed by diagnostics.
+  unsigned getAddressSpaceAttributePrintValue() const {
+auto Addr = getAddressSpace();
+// This function is not supposed to be used with language specific
+// address spaces. If that happens, the diagnostic message should consider
+// printing the QualType instead of the address space value.
+assert(Addr == 0 || Addr >= LangAS::Count);
+if (Addr)
+  return Addr - LangAS::Count;
+// TODO: The diagnostic messages where Addr may be 0 should be fixed
+// since it cannot differentiate the situation where 0 denotes the default
+// address space or user specified __attribute__((address_space(0))).
+return 0;
+  }
   void setAddressSpace(unsigned space) {
 assert(space <= MaxAddressSpace);
 Mask = (Mask & ~AddressSpaceMask)
Index: cfe/trunk/include/clang/AST/ASTContext.h
===
--- cfe/trunk/include/clang/AST/ASTContext.h
+++ cfe/trunk/include/clang/AST/ASTContext.h
@@ -2317,21 +2317,15 @@
 return getTargetAddressSpace(Q.getAddressSpace());
   }
 
-  unsigned getTargetAddressSpace(unsigned AS) const {
-if (AS < LangAS::Offset || AS >= LangAS::Offset + LangAS::Count)
-  return AS;
-else
-  return (*AddrSpaceMap)[AS - LangAS::Offset];
-  }
+  unsigned getTargetAddressSpace(unsigned AS) const;
 
   /// Get target-dependent integer value for null pointer which is used for
   /// constant folding.
   uint64_t getTargetNullPointerValue(QualType QT) const;
 
   bool addressSpaceMapManglingFor(unsigned AS) const {
-return AddrSpaceMapMangling ||
-   AS < LangAS::Offset ||
-   AS >= LangAS::Offset + LangAS::Count;
+return AddrSpaceMapMangling || 
+   AS >= LangAS::Count;
   }
 
 private:
Index: cfe/trunk/include/clang/Basic/AddressSpaces.h
===
--- cfe/trunk/include/clang/Basic/AddressSpaces.h
+++ cfe/trunk/include/clang/Basic/AddressSpaces.h
@@ -20,24 +20,32 @@
 
 namespace LangAS {
 
-/// \brief Defines the set of possible language-specific address spaces.
+/// \brief Defines the address space values used by the address space qualifier
+/// of QualType.
 ///
-/// This uses a high starting offset so as not to conflict with any address
-/// space used by a target.
 enum ID {
-  Offset = 0x7FFF00,
+  // The default value 0 is the value used in QualType for the the situation
+  // where there is no address space qualifier. For most languages, this also
+  // corresponds to the situation where there is no address space qualifier in
+  // the source code, except for OpenCL, where the address space value 0 in
+  // QualType represents private address space in OpenCL source code.
+  Default = 0,
 
-  opencl_global = Offset,
+  // OpenCL specific address spaces.
+  opencl_global,
   opencl_local,
   opencl_constant,
   opencl_generic,
 
+  // CUDA specific address spaces.
   cuda_device,
   cuda_constant,
   cuda_shared,
 
-  Last,
-  Count = Last-Offset
+  // This denotes the count of language-specific address spaces and also
+  // the offset added to the target-specific address spaces, which are usually
+  // specified by address space attributes __attribute__(address_space(n))).
+  Count
 };
 
 /// The type of a lookup table which maps from language-specific address spaces
Index: cfe/trunk/test/SemaOpenCL/invalid-assignment-constant-address-space.cl

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia accepted this revision.
Anastasia added a comment.

LGTM! Thanks!


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

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

Any further comments? Thanks.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

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



Comment at: include/clang/Basic/AddressSpaces.h:32
+  // QualType represents private address space in OpenCL source code.
+  Default = 0,
+

Anastasia wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > The alloca AS is not taken from the target AS map but from the 
> > > DataLayout. This keep me wonder whether the explicit Default item is 
> > > actually needed here
> > For OpenCL, the default addr space is mapped to alloca addr space. For 
> > other languages, it is mapped by the address space mapping table.
> Ok. BTW, why is it done differently for other languages than OpenCL now? Is 
> it something we missed in the programming model before? Or is it something 
> specific to AMD target support?
This is because amdgcn target now supports a new address space mapping where 
alloca address space is different from generic address space. For OpenCL, 
address space 0 should be mapped to alloca address space; for other languages, 
address space 0 should be mapped to a target specific generic address space. 
The previous approach for mapping address spaces cannot handle the requirements 
since it always maps address space 0 to target address space 0. Therefore it 
needs to be extended for more general cases.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: include/clang/Basic/AddressSpaces.h:32
+  // QualType represents private address space in OpenCL source code.
+  Default = 0,
+

yaxunl wrote:
> Anastasia wrote:
> > The alloca AS is not taken from the target AS map but from the DataLayout. 
> > This keep me wonder whether the explicit Default item is actually needed 
> > here
> For OpenCL, the default addr space is mapped to alloca addr space. For other 
> languages, it is mapped by the address space mapping table.
Ok. BTW, why is it done differently for other languages than OpenCL now? Is it 
something we missed in the programming model before? Or is it something 
specific to AMD target support?


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl updated this revision to Diff 94399.
yaxunl marked 5 inline comments as done.
yaxunl added a comment.

Revised by Anastasia's comments.


https://reviews.llvm.org/D31404

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  include/clang/Basic/AddressSpaces.h
  lib/AST/ASTContext.cpp
  lib/AST/ExprClassification.cpp
  lib/AST/TypePrinter.cpp
  lib/Basic/Targets.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGen/address-space.c
  test/CodeGen/default-address-space.c
  test/CodeGenOpenCL/address-space-constant-initializers.cl
  test/CodeGenOpenCL/address-spaces.cl
  test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
  test/CodeGenOpenCL/vla.cl
  test/Sema/address_spaces.c
  test/Sema/invalid-assignment-constant-address-space.c
  test/SemaOpenCL/invalid-assignment-constant-address-space.cl

Index: test/SemaOpenCL/invalid-assignment-constant-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCL/invalid-assignment-constant-address-space.cl
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+int constant c[3] = {0};
+
+void foo() {
+  c[0] = 1; //expected-error{{read-only variable is not assignable}}
+}
Index: test/Sema/invalid-assignment-constant-address-space.c
===
--- test/Sema/invalid-assignment-constant-address-space.c
+++ /dev/null
@@ -1,8 +0,0 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
-
-#define OPENCL_CONSTANT 8388354
-int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};
-
-void foo() {
-  c[0] = 1; //expected-error{{read-only variable is not assignable}}
-}
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -20,7 +20,7 @@
   _AS1 int arrarr[5][5]; // expected-error {{automatic variable qualified with an address space}}
 
   __attribute__((address_space(-1))) int *_boundsA; // expected-error {{address space is negative}}
-  __attribute__((address_space(0x7F))) int *_boundsB;
+  __attribute__((address_space(0x7F))) int *_boundsB; // expected-error {{address space is larger than the maximum supported}}
   __attribute__((address_space(0x100))) int *_boundsC; // expected-error {{address space is larger than the maximum supported}}
   // chosen specifically to overflow 32 bits and come out reasonable
   __attribute__((address_space(4294967500))) int *_boundsD; // expected-error {{address space is larger than the maximum supported}}
Index: test/CodeGenOpenCL/vla.cl
===
--- test/CodeGenOpenCL/vla.cl
+++ test/CodeGenOpenCL/vla.cl
@@ -1,18 +1,26 @@
-// RUN: %clang_cc1 -emit-llvm -triple "spir-unknown-unknown" -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple "spir-unknown-unknown" -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 -emit-llvm -triple amdgcn-amd-amdhsa-opencl -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 -emit-llvm -triple amdgcn-amd-amdhsa-amdgizcl -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,GIZ %s
 
 constant int sz0 = 5;
-// CHECK: @sz0 = addrspace(2) constant i32 5
+// SPIR: @sz0 = addrspace(2) constant i32 5
+// GIZ: @sz0 = addrspace(4) constant i32 5
 const global int sz1 = 16;
 // CHECK: @sz1 = addrspace(1) constant i32 16
 const constant int sz2 = 8;
-// CHECK: @sz2 = addrspace(2) constant i32 8
+// SPIR: @sz2 = addrspace(2) constant i32 8
+// GIZ: @sz2 = addrspace(4) constant i32 8
 // CHECK: @testvla.vla2 = internal addrspace(3) global [8 x i16] undef
 
 kernel void testvla()
 {
   int vla0[sz0];
-// CHECK: %vla0 = alloca [5 x i32]
+// SPIR: %vla0 = alloca [5 x i32]
+// SPIR-NOT: %vla0 = alloca [5 x i32]{{.*}}addrspace
+// GIZ: %vla0 = alloca [5 x i32]{{.*}}addrspace(5)
   char vla1[sz1];
-// CHECK: %vla1 = alloca [16 x i8]
+// SPIR: %vla1 = alloca [16 x i8]
+// SPIR-NOT: %vla1 = alloca [16 x i8]{{.*}}addrspace
+// GIZ: %vla1 = alloca [16 x i8]{{.*}}addrspace(5)
   local short vla2[sz2];
 }
Index: test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
===
--- test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
+++ test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
@@ -4,6 +4,6 @@
 // RUN: %clang_cc1 %s -O0 -triple amdgcn---amdgizcl -emit-llvm -o - | FileCheck -check-prefix=GIZ %s
 
 // CHECK: target datalayout = "e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
-// GIZ: target datalayout = "e-p:64:64-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
+// GIZ: target 

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/ASTContext.h:2328
+return AddrSpaceMapMangling || 
+   AS >= LangAS::target_first;
   }

Anastasia wrote:
> yaxunl wrote:
> > yaxunl wrote:
> > > Anastasia wrote:
> > > > Anastasia wrote:
> > > > > yaxunl wrote:
> > > > > > Anastasia wrote:
> > > > > > > So we couldn't use the  LangAS::Count instead?
> > > > > > > 
> > > > > > > I have the same comment in other places that use 
> > > > > > > LangAS::target_first. Why couldn't we simply use LangAS::Count? 
> > > > > > > It there any point in having two tags?
> > > > > > > 
> > > > > > > Another comment is why do we need ASes specified by 
> > > > > > > `__attribute__((address_space(n)))` to be unique enum number at 
> > > > > > > the end of named ASes of OpenCL and CUDA? I think conceptually 
> > > > > > > the full range of ASes can be used in C because the ASes from 
> > > > > > > OpenCL and CUDA are not available there anyways.
> > > > > > I will use LangAS::Count instead and remove target_first, since 
> > > > > > their values are the same.
> > > > > > 
> > > > > > For your second question:  the values for 
> > > > > > `__attribute__((address_space(n)))` need to be different from the 
> > > > > > language specific address space values because they are mapped to 
> > > > > > target address space differently.
> > > > > > 
> > > > > > For language specific address space, they are mapped through a 
> > > > > > target defined mapping table.
> > > > > > 
> > > > > > For `__attribute__((address_space(n)))`, the target address space 
> > > > > > should be the same as n, without going through the mapping table.
> > > > > > 
> > > > > > If they are defined in overlapping value ranges, they cannot be 
> > > > > > handled in different ways.
> > > > > > 
> > > > > > 
> > > > > Target address space map currently corresponds to the named address 
> > > > > spaces of OpenCL and CUDA only. So if the idea is to avoid 
> > > > > overlapping with those we should extend the table? Although, I don't 
> > > > > see how this can be done because it will require fixed semantic of 
> > > > > address spaces in C which is currently undefined.
> > > > Perhaps I am missing something but I don't see any change here that 
> > > > makes non-named address spaces follow different path for the target.
> > > > 
> > > > Also does this change relate to alloca extension in some way? I still 
> > > > struggle to understand this fully...
> > > > 
> > > > All I can see is that this change restricts overlapping of named and 
> > > > non-named address spaces but I can't clearly understand the motivation 
> > > > for this.
> > > `__attribute__((address_space(n)))` is used in C and C++ to specify 
> > > target address space for a variable. 
> > > 
> > > For example, 
> > > https://github.com/llvm-mirror/clang/blob/master/test/Sema/address_spaces.c
> > >  
> > > 
> > > Many cases they just need to put a variable in certain target address 
> > > space and do not need specific semantics for these address spaces.
> > In the definition of `ASTContext::getTargetAddressSpace()` you can see how 
> > address space values below and above LangAS::Count are handled differently.
> > 
> > The old definition of address space enum does not differentiate between the 
> > default address space 0 (no address space qualifier) and 
> > `__attribute__((address_space(0)))`.
> > 
> > Before alloca API changes, this does not matter, since address space 0 in 
> > AST is always mapped to target address space 0.
> > 
> > However, after alloca API changes, default address space 0 is mapped to 
> > target alloca address space, which may be non-zero. Therefore it is 
> > necessary to differentiate between `__attribute__((address_space(0)))` and 
> > the default address space 0. Otherwise, if a user specifies 
> > `__attribute__((address_space(0)))`, it may not be mapped to target address 
> > space 0.
> > 
> > 
> > However, after alloca API changes, default address space 0 is mapped to 
> > target alloca address space, which may be non-zero. Therefore it is 
> > necessary to differentiate between __attribute__((address_space(0))) and 
> > the default address space 0. 
>  In your patch the default AS corresponds to value set in the target address 
> space map and not taken from the alloca AS of the DataLayout.
Addr space 0 is mapped to alloca addr space for OpenCL and mapped to 
target-specified addr space for other languages, which is done by 
ASTContext::getTargetAddressSpace.

In either case, it may not be mapped to 0.



Comment at: include/clang/Basic/AddressSpaces.h:32
+  // QualType represents private address space in OpenCL source code.
+  Default = 0,
+

Anastasia wrote:
> The alloca AS is not taken from the target AS map but from the DataLayout. 
> This keep me wonder whether the explicit Default item is actually needed 
> here
For OpenCL, 

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: include/clang/AST/ASTContext.h:2328
+return AddrSpaceMapMangling || 
+   AS >= LangAS::target_first;
   }

yaxunl wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > Anastasia wrote:
> > > > yaxunl wrote:
> > > > > Anastasia wrote:
> > > > > > So we couldn't use the  LangAS::Count instead?
> > > > > > 
> > > > > > I have the same comment in other places that use 
> > > > > > LangAS::target_first. Why couldn't we simply use LangAS::Count? It 
> > > > > > there any point in having two tags?
> > > > > > 
> > > > > > Another comment is why do we need ASes specified by 
> > > > > > `__attribute__((address_space(n)))` to be unique enum number at the 
> > > > > > end of named ASes of OpenCL and CUDA? I think conceptually the full 
> > > > > > range of ASes can be used in C because the ASes from OpenCL and 
> > > > > > CUDA are not available there anyways.
> > > > > I will use LangAS::Count instead and remove target_first, since their 
> > > > > values are the same.
> > > > > 
> > > > > For your second question:  the values for 
> > > > > `__attribute__((address_space(n)))` need to be different from the 
> > > > > language specific address space values because they are mapped to 
> > > > > target address space differently.
> > > > > 
> > > > > For language specific address space, they are mapped through a target 
> > > > > defined mapping table.
> > > > > 
> > > > > For `__attribute__((address_space(n)))`, the target address space 
> > > > > should be the same as n, without going through the mapping table.
> > > > > 
> > > > > If they are defined in overlapping value ranges, they cannot be 
> > > > > handled in different ways.
> > > > > 
> > > > > 
> > > > Target address space map currently corresponds to the named address 
> > > > spaces of OpenCL and CUDA only. So if the idea is to avoid overlapping 
> > > > with those we should extend the table? Although, I don't see how this 
> > > > can be done because it will require fixed semantic of address spaces in 
> > > > C which is currently undefined.
> > > Perhaps I am missing something but I don't see any change here that makes 
> > > non-named address spaces follow different path for the target.
> > > 
> > > Also does this change relate to alloca extension in some way? I still 
> > > struggle to understand this fully...
> > > 
> > > All I can see is that this change restricts overlapping of named and 
> > > non-named address spaces but I can't clearly understand the motivation 
> > > for this.
> > `__attribute__((address_space(n)))` is used in C and C++ to specify target 
> > address space for a variable. 
> > 
> > For example, 
> > https://github.com/llvm-mirror/clang/blob/master/test/Sema/address_spaces.c 
> > 
> > Many cases they just need to put a variable in certain target address space 
> > and do not need specific semantics for these address spaces.
> In the definition of `ASTContext::getTargetAddressSpace()` you can see how 
> address space values below and above LangAS::Count are handled differently.
> 
> The old definition of address space enum does not differentiate between the 
> default address space 0 (no address space qualifier) and 
> `__attribute__((address_space(0)))`.
> 
> Before alloca API changes, this does not matter, since address space 0 in AST 
> is always mapped to target address space 0.
> 
> However, after alloca API changes, default address space 0 is mapped to 
> target alloca address space, which may be non-zero. Therefore it is necessary 
> to differentiate between `__attribute__((address_space(0)))` and the default 
> address space 0. Otherwise, if a user specifies 
> `__attribute__((address_space(0)))`, it may not be mapped to target address 
> space 0.
> 
> 
> However, after alloca API changes, default address space 0 is mapped to 
> target alloca address space, which may be non-zero. Therefore it is necessary 
> to differentiate between __attribute__((address_space(0))) and the default 
> address space 0. 
 In your patch the default AS corresponds to value set in the target address 
space map and not taken from the alloca AS of the DataLayout.



Comment at: include/clang/Basic/AddressSpaces.h:32
+  // QualType represents private address space in OpenCL source code.
+  Default = 0,
+

The alloca AS is not taken from the target AS map but from the DataLayout. This 
keep me wonder whether the explicit Default item is actually needed here



Comment at: lib/Basic/Targets.cpp:8382
 static const unsigned SPIRAddrSpaceMap[] = {
+4, // Default
 1, // opencl_global

This will break use case of compiling for SPIR from C which is needed by some 
frameworks.
We can only use generic if compiled in OpenCL2.0 mode and only for pointer 
types.



Comment at: lib/Sema/SemaExprCXX.cpp:3121
 
-if (unsigned AddressSpace = Pointee.getAddressSpace())
+if 

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

Ping! Any further questions? Thanks.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

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

Revised by Anastasia's comments.
Add lit test for `__attribute__((address_space(0)))` and default address space.


https://reviews.llvm.org/D31404

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  include/clang/Basic/AddressSpaces.h
  lib/AST/ASTContext.cpp
  lib/AST/ExprClassification.cpp
  lib/AST/TypePrinter.cpp
  lib/Basic/Targets.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGen/address-space.c
  test/CodeGen/default-address-space.c
  test/CodeGenOpenCL/address-space-constant-initializers.cl
  test/CodeGenOpenCL/address-spaces.cl
  test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
  test/CodeGenOpenCL/vla.cl
  test/Sema/address_spaces.c
  test/Sema/invalid-assignment-constant-address-space.c
  test/SemaOpenCL/invalid-assignment-constant-address-space.cl

Index: test/SemaOpenCL/invalid-assignment-constant-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCL/invalid-assignment-constant-address-space.cl
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+int constant c[3] = {0};
+
+void foo() {
+  c[0] = 1; //expected-error{{read-only variable is not assignable}}
+}
Index: test/Sema/invalid-assignment-constant-address-space.c
===
--- test/Sema/invalid-assignment-constant-address-space.c
+++ /dev/null
@@ -1,8 +0,0 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
-
-#define OPENCL_CONSTANT 8388354
-int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};
-
-void foo() {
-  c[0] = 1; //expected-error{{read-only variable is not assignable}}
-}
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -20,7 +20,7 @@
   _AS1 int arrarr[5][5]; // expected-error {{automatic variable qualified with an address space}}
 
   __attribute__((address_space(-1))) int *_boundsA; // expected-error {{address space is negative}}
-  __attribute__((address_space(0x7F))) int *_boundsB;
+  __attribute__((address_space(0x7F))) int *_boundsB; // expected-error {{address space is larger than the maximum supported}}
   __attribute__((address_space(0x100))) int *_boundsC; // expected-error {{address space is larger than the maximum supported}}
   // chosen specifically to overflow 32 bits and come out reasonable
   __attribute__((address_space(4294967500))) int *_boundsD; // expected-error {{address space is larger than the maximum supported}}
Index: test/CodeGenOpenCL/vla.cl
===
--- test/CodeGenOpenCL/vla.cl
+++ test/CodeGenOpenCL/vla.cl
@@ -1,18 +1,26 @@
-// RUN: %clang_cc1 -emit-llvm -triple "spir-unknown-unknown" -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple "spir-unknown-unknown" -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 -emit-llvm -triple amdgcn-amd-amdhsa-opencl -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 -emit-llvm -triple amdgcn-amd-amdhsa-amdgizcl -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,GIZ %s
 
 constant int sz0 = 5;
-// CHECK: @sz0 = addrspace(2) constant i32 5
+// SPIR: @sz0 = addrspace(2) constant i32 5
+// GIZ: @sz0 = addrspace(4) constant i32 5
 const global int sz1 = 16;
 // CHECK: @sz1 = addrspace(1) constant i32 16
 const constant int sz2 = 8;
-// CHECK: @sz2 = addrspace(2) constant i32 8
+// SPIR: @sz2 = addrspace(2) constant i32 8
+// GIZ: @sz2 = addrspace(4) constant i32 8
 // CHECK: @testvla.vla2 = internal addrspace(3) global [8 x i16] undef
 
 kernel void testvla()
 {
   int vla0[sz0];
-// CHECK: %vla0 = alloca [5 x i32]
+// SPIR: %vla0 = alloca [5 x i32]
+// SPIR-NOT: %vla0 = alloca [5 x i32]{{.*}}addrspace
+// GIZ: %vla0 = alloca [5 x i32]{{.*}}addrspace(5)
   char vla1[sz1];
-// CHECK: %vla1 = alloca [16 x i8]
+// SPIR: %vla1 = alloca [16 x i8]
+// SPIR-NOT: %vla1 = alloca [16 x i8]{{.*}}addrspace
+// GIZ: %vla1 = alloca [16 x i8]{{.*}}addrspace(5)
   local short vla2[sz2];
 }
Index: test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
===
--- test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
+++ test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
@@ -4,6 +4,6 @@
 // RUN: %clang_cc1 %s -O0 -triple amdgcn---amdgizcl -emit-llvm -o - | FileCheck -check-prefix=GIZ %s
 
 // CHECK: target datalayout = "e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
-// GIZ: target datalayout = 

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/ASTContext.h:2328
+return AddrSpaceMapMangling || 
+   AS >= LangAS::target_first;
   }

Anastasia wrote:
> Anastasia wrote:
> > yaxunl wrote:
> > > Anastasia wrote:
> > > > So we couldn't use the  LangAS::Count instead?
> > > > 
> > > > I have the same comment in other places that use LangAS::target_first. 
> > > > Why couldn't we simply use LangAS::Count? It there any point in having 
> > > > two tags?
> > > > 
> > > > Another comment is why do we need ASes specified by 
> > > > `__attribute__((address_space(n)))` to be unique enum number at the end 
> > > > of named ASes of OpenCL and CUDA? I think conceptually the full range 
> > > > of ASes can be used in C because the ASes from OpenCL and CUDA are not 
> > > > available there anyways.
> > > I will use LangAS::Count instead and remove target_first, since their 
> > > values are the same.
> > > 
> > > For your second question:  the values for 
> > > `__attribute__((address_space(n)))` need to be different from the 
> > > language specific address space values because they are mapped to target 
> > > address space differently.
> > > 
> > > For language specific address space, they are mapped through a target 
> > > defined mapping table.
> > > 
> > > For `__attribute__((address_space(n)))`, the target address space should 
> > > be the same as n, without going through the mapping table.
> > > 
> > > If they are defined in overlapping value ranges, they cannot be handled 
> > > in different ways.
> > > 
> > > 
> > Target address space map currently corresponds to the named address spaces 
> > of OpenCL and CUDA only. So if the idea is to avoid overlapping with those 
> > we should extend the table? Although, I don't see how this can be done 
> > because it will require fixed semantic of address spaces in C which is 
> > currently undefined.
> Perhaps I am missing something but I don't see any change here that makes 
> non-named address spaces follow different path for the target.
> 
> Also does this change relate to alloca extension in some way? I still 
> struggle to understand this fully...
> 
> All I can see is that this change restricts overlapping of named and 
> non-named address spaces but I can't clearly understand the motivation for 
> this.
`__attribute__((address_space(n)))` is used in C and C++ to specify target 
address space for a variable. 

For example, 
https://github.com/llvm-mirror/clang/blob/master/test/Sema/address_spaces.c 

Many cases they just need to put a variable in certain target address space and 
do not need specific semantics for these address spaces.



Comment at: include/clang/AST/ASTContext.h:2328
+return AddrSpaceMapMangling || 
+   AS >= LangAS::target_first;
   }

yaxunl wrote:
> Anastasia wrote:
> > Anastasia wrote:
> > > yaxunl wrote:
> > > > Anastasia wrote:
> > > > > So we couldn't use the  LangAS::Count instead?
> > > > > 
> > > > > I have the same comment in other places that use 
> > > > > LangAS::target_first. Why couldn't we simply use LangAS::Count? It 
> > > > > there any point in having two tags?
> > > > > 
> > > > > Another comment is why do we need ASes specified by 
> > > > > `__attribute__((address_space(n)))` to be unique enum number at the 
> > > > > end of named ASes of OpenCL and CUDA? I think conceptually the full 
> > > > > range of ASes can be used in C because the ASes from OpenCL and CUDA 
> > > > > are not available there anyways.
> > > > I will use LangAS::Count instead and remove target_first, since their 
> > > > values are the same.
> > > > 
> > > > For your second question:  the values for 
> > > > `__attribute__((address_space(n)))` need to be different from the 
> > > > language specific address space values because they are mapped to 
> > > > target address space differently.
> > > > 
> > > > For language specific address space, they are mapped through a target 
> > > > defined mapping table.
> > > > 
> > > > For `__attribute__((address_space(n)))`, the target address space 
> > > > should be the same as n, without going through the mapping table.
> > > > 
> > > > If they are defined in overlapping value ranges, they cannot be handled 
> > > > in different ways.
> > > > 
> > > > 
> > > Target address space map currently corresponds to the named address 
> > > spaces of OpenCL and CUDA only. So if the idea is to avoid overlapping 
> > > with those we should extend the table? Although, I don't see how this can 
> > > be done because it will require fixed semantic of address spaces in C 
> > > which is currently undefined.
> > Perhaps I am missing something but I don't see any change here that makes 
> > non-named address spaces follow different path for the target.
> > 
> > Also does this change relate to alloca extension in some way? I still 
> > struggle to understand this fully...
> > 
> > All 

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: include/clang/AST/ASTContext.h:2328
+return AddrSpaceMapMangling || 
+   AS >= LangAS::target_first;
   }

Anastasia wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > So we couldn't use the  LangAS::Count instead?
> > > 
> > > I have the same comment in other places that use LangAS::target_first. 
> > > Why couldn't we simply use LangAS::Count? It there any point in having 
> > > two tags?
> > > 
> > > Another comment is why do we need ASes specified by 
> > > `__attribute__((address_space(n)))` to be unique enum number at the end 
> > > of named ASes of OpenCL and CUDA? I think conceptually the full range of 
> > > ASes can be used in C because the ASes from OpenCL and CUDA are not 
> > > available there anyways.
> > I will use LangAS::Count instead and remove target_first, since their 
> > values are the same.
> > 
> > For your second question:  the values for 
> > `__attribute__((address_space(n)))` need to be different from the language 
> > specific address space values because they are mapped to target address 
> > space differently.
> > 
> > For language specific address space, they are mapped through a target 
> > defined mapping table.
> > 
> > For `__attribute__((address_space(n)))`, the target address space should be 
> > the same as n, without going through the mapping table.
> > 
> > If they are defined in overlapping value ranges, they cannot be handled in 
> > different ways.
> > 
> > 
> Target address space map currently corresponds to the named address spaces of 
> OpenCL and CUDA only. So if the idea is to avoid overlapping with those we 
> should extend the table? Although, I don't see how this can be done because 
> it will require fixed semantic of address spaces in C which is currently 
> undefined.
Perhaps I am missing something but I don't see any change here that makes 
non-named address spaces follow different path for the target.

Also does this change relate to alloca extension in some way? I still struggle 
to understand this fully...

All I can see is that this change restricts overlapping of named and non-named 
address spaces but I can't clearly understand the motivation for this.



Comment at: lib/Basic/Targets.cpp:2035
 static const LangAS::Map AMDGPUPrivateIsZeroMap = {
+4,  // Default
 1,  // opencl_global

This should use address space attribute 4 for all non-AS type objects. Could we 
make sure we have a codegen test for this? 


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: include/clang/AST/ASTContext.h:2328
+return AddrSpaceMapMangling || 
+   AS >= LangAS::target_first;
   }

yaxunl wrote:
> Anastasia wrote:
> > So we couldn't use the  LangAS::Count instead?
> > 
> > I have the same comment in other places that use LangAS::target_first. Why 
> > couldn't we simply use LangAS::Count? It there any point in having two tags?
> > 
> > Another comment is why do we need ASes specified by 
> > `__attribute__((address_space(n)))` to be unique enum number at the end of 
> > named ASes of OpenCL and CUDA? I think conceptually the full range of ASes 
> > can be used in C because the ASes from OpenCL and CUDA are not available 
> > there anyways.
> I will use LangAS::Count instead and remove target_first, since their values 
> are the same.
> 
> For your second question:  the values for `__attribute__((address_space(n)))` 
> need to be different from the language specific address space values because 
> they are mapped to target address space differently.
> 
> For language specific address space, they are mapped through a target defined 
> mapping table.
> 
> For `__attribute__((address_space(n)))`, the target address space should be 
> the same as n, without going through the mapping table.
> 
> If they are defined in overlapping value ranges, they cannot be handled in 
> different ways.
> 
> 
Target address space map currently corresponds to the named address spaces of 
OpenCL and CUDA only. So if the idea is to avoid overlapping with those we 
should extend the table? Although, I don't see how this can be done because it 
will require fixed semantic of address spaces in C which is currently undefined.



Comment at: include/clang/AST/Type.h:345
+  return Addr - LangAS::Count;
+// ToDo: The diagnostic messages where Addr may be 0 should be fixed
+// since it cannot differentiate the situation where 0 denotes the default

ToDo -> TODO



Comment at: lib/AST/ASTContext.cpp:9556
+  if (AS == LangAS::Default && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();
+  if (AS >= LangAS::target_first)

yaxunl wrote:
> Anastasia wrote:
> > t-tye wrote:
> > > An alternative to doing this would be to add an opencl_private to LangAS 
> > > and have each target map it accordingly. Then this could be:
> > > 
> > > ```
> > > // If a target specific address space was specified, simply return it.
> > > if (AS >= LangAS::target_first)
> > >   return AS - LangAS::target_first;
> > > // For OpenCL, only function local variables are not explicitly marked 
> > > with
> > > // an address space in the AST, so treat them as the OpenCL private 
> > > address space.
> > > if (!AS && LangOpts.OpenCL)
> > >   AS = LangAS::opencl_private;
> > > return (*AddrSpaceMap)[AS];
> > > ```
> > > This seems to better express what is happening here. If no address space 
> > > was specified, and the language is OpenCL, then treat it as OpenCL 
> > > private and map it according to the target mapping.
> > > 
> > > If wanted to eliminate the LangAS::Default named constant then that would 
> > > be possible as it is no longer being used by name. However, would need to 
> > > ensure that the first named enumerators starts at 1 so that 0 is left as 
> > > the "no value explicitly specified" value that each target must map to 
> > > the target specific generic address space.
> > I would very much like to see `opencl_private` represented explicitly. This 
> > would allow us to simplify some parsing and also enable proper support of 
> > `NULL ` literal (that has no AS by the spec).
> > 
> > We can of course do this refactoring work as a separate step.
> Introducing opencl_private could incur quite a few changes to AST, Sema, and 
> lit tests.
> 
> We'd better do that in another patch since the main objective of this patch 
> is to get Clang codegen work with the new alloca API and non-zero private 
> address space.
> 
Makes sense!



Comment at: lib/Sema/SemaExprCXX.cpp:3121
 
-if (unsigned AddressSpace = Pointee.getAddressSpace())
+if (Pointee.getQualifiers().getAddressSpace())
   return Diag(Ex.get()->getLocStart(),

`Pointee.getAddressSpace()` wouldn't work any more?


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Tony Tye via Phabricator via cfe-commits]

t-tye accepted this revision.
t-tye added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.



Comment at: include/clang/AST/Type.h:339-340
+auto Addr = getAddressSpace();
+if (Addr == 0)
+  return 0;
+return Addr - LangAS::target_first;

Anastasia wrote:
> yaxunl wrote:
> > t-tye wrote:
> > > Since you mention this is only used for  
> > > `__attribute__((address_space(n)))`, why is this check for 0 needed?
> > > 
> > > If it is needed then to match other places should it simply be:
> > > 
> > > ```
> > > if (Addr)
> > >   return Addr - LangAS::target_first;
> > > return 0;
> > > ```
> > It is for `__attribute__((address_space(n)))` and the default addr space 0.
> > 
> > For the default addr space 0, we want to print 0 instead of 
> > `-LangAS::target_first`.
> > 
> > I will make the change for matching other places.
> Could we use LangAS::Count instead?
I do not think the address space 0 should be returned as 0 as then it is 
impossible to distinguish between a type that has no address space attribute, 
and one that has an explicit address space attribute with the value 0.

But that seems to be a bug in the original code so I would suggest leaving this 
for now and fixing it as a separate patch. The diagnostic message should really 
be checking if an address space attribute was present (by checking for 0), and 
changing the working of the message accordingly.

Suggest add a TODO here to mention this which can be fixed in a later patch.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

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



Comment at: include/clang/AST/ASTContext.h:2328
+return AddrSpaceMapMangling || 
+   AS >= LangAS::target_first;
   }

Anastasia wrote:
> So we couldn't use the  LangAS::Count instead?
> 
> I have the same comment in other places that use LangAS::target_first. Why 
> couldn't we simply use LangAS::Count? It there any point in having two tags?
> 
> Another comment is why do we need ASes specified by 
> `__attribute__((address_space(n)))` to be unique enum number at the end of 
> named ASes of OpenCL and CUDA? I think conceptually the full range of ASes 
> can be used in C because the ASes from OpenCL and CUDA are not available 
> there anyways.
I will use LangAS::Count instead and remove target_first, since their values 
are the same.

For your second question:  the values for `__attribute__((address_space(n)))` 
need to be different from the language specific address space values because 
they are mapped to target address space differently.

For language specific address space, they are mapped through a target defined 
mapping table.

For `__attribute__((address_space(n)))`, the target address space should be the 
same as n, without going through the mapping table.

If they are defined in overlapping value ranges, they cannot be handled in 
different ways.





Comment at: include/clang/AST/Type.h:339-340
+auto Addr = getAddressSpace();
+if (Addr == 0)
+  return 0;
+return Addr - LangAS::target_first;

t-tye wrote:
> Since you mention this is only used for  `__attribute__((address_space(n)))`, 
> why is this check for 0 needed?
> 
> If it is needed then to match other places should it simply be:
> 
> ```
> if (Addr)
>   return Addr - LangAS::target_first;
> return 0;
> ```
It is for `__attribute__((address_space(n)))` and the default addr space 0.

For the default addr space 0, we want to print 0 instead of 
`-LangAS::target_first`.

I will make the change for matching other places.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2451-2453
+def warn_attribute_address_space_negative : Warning<
+  "address space is negative">,
+  InGroup>;

t-tye wrote:
> Now the one questionable test has been fixed, should the handling of 
> address_space attribute go back to it being an error if n is negative? That 
> seems more logical.
Will do



Comment at: lib/AST/ASTContext.cpp:9555
+  // alloca.
+  if (AS == LangAS::Default && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();

t-tye wrote:
> To be consistent with other places should this simply be:
> ```
> if (!AS && LangOpts.OpenCL)
> ```
Will do.



Comment at: lib/AST/ASTContext.cpp:9556
+  if (AS == LangAS::Default && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();
+  if (AS >= LangAS::target_first)

Anastasia wrote:
> t-tye wrote:
> > An alternative to doing this would be to add an opencl_private to LangAS 
> > and have each target map it accordingly. Then this could be:
> > 
> > ```
> > // If a target specific address space was specified, simply return it.
> > if (AS >= LangAS::target_first)
> >   return AS - LangAS::target_first;
> > // For OpenCL, only function local variables are not explicitly marked with
> > // an address space in the AST, so treat them as the OpenCL private address 
> > space.
> > if (!AS && LangOpts.OpenCL)
> >   AS = LangAS::opencl_private;
> > return (*AddrSpaceMap)[AS];
> > ```
> > This seems to better express what is happening here. If no address space 
> > was specified, and the language is OpenCL, then treat it as OpenCL private 
> > and map it according to the target mapping.
> > 
> > If wanted to eliminate the LangAS::Default named constant then that would 
> > be possible as it is no longer being used by name. However, would need to 
> > ensure that the first named enumerators starts at 1 so that 0 is left as 
> > the "no value explicitly specified" value that each target must map to the 
> > target specific generic address space.
> I would very much like to see `opencl_private` represented explicitly. This 
> would allow us to simplify some parsing and also enable proper support of 
> `NULL ` literal (that has no AS by the spec).
> 
> We can of course do this refactoring work as a separate step.
Introducing opencl_private could incur quite a few changes to AST, Sema, and 
lit tests.

We'd better do that in another patch since the main objective of this patch is 
to get Clang codegen work with the new alloca API and non-zero private address 
space.




Comment at: lib/Sema/SemaExprCXX.cpp:2055
+  << AllocType.getUnqualifiedType()
+  << AllocType.getQualifiers().getAddressSpacePrintValue();
   else if (getLangOpts().ObjCAutoRefCount) {

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.



Comment at: lib/Sema/SemaExprCXX.cpp:2055
+  << AllocType.getUnqualifiedType()
+  << AllocType.getQualifiers().getAddressSpacePrintValue();
   else if (getLangOpts().ObjCAutoRefCount) {

Would suggest renaming getAddressSpacePrintValue to 
getAddressSpaceAttributePrintValue since it only deals with address spaces 
coming from the` __attribute__((address_space(n)))`.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added inline comments.



Comment at: include/clang/AST/ASTContext.h:2328
+return AddrSpaceMapMangling || 
+   AS >= LangAS::target_first;
   }

So we couldn't use the  LangAS::Count instead?

I have the same comment in other places that use LangAS::target_first. Why 
couldn't we simply use LangAS::Count? It there any point in having two tags?

Another comment is why do we need ASes specified by 
`__attribute__((address_space(n)))` to be unique enum number at the end of 
named ASes of OpenCL and CUDA? I think conceptually the full range of ASes can 
be used in C because the ASes from OpenCL and CUDA are not available there 
anyways.



Comment at: include/clang/AST/Type.h:339-340
+auto Addr = getAddressSpace();
+if (Addr == 0)
+  return 0;
+return Addr - LangAS::target_first;

t-tye wrote:
> Since you mention this is only used for  `__attribute__((address_space(n)))`, 
> why is this check for 0 needed?
> 
> If it is needed then to match other places should it simply be:
> 
> ```
> if (Addr)
>   return Addr - LangAS::target_first;
> return 0;
> ```
Could we use LangAS::Count instead?



Comment at: lib/AST/ASTContext.cpp:8732
+Type = Context.getAddrSpaceQualType(Type, AddrSpace +
+LangAS::target_first);
 Str = End;

Also here, could we use LangAS::Count instead?



Comment at: lib/AST/ASTContext.cpp:9556
+  if (AS == LangAS::Default && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();
+  if (AS >= LangAS::target_first)

t-tye wrote:
> An alternative to doing this would be to add an opencl_private to LangAS and 
> have each target map it accordingly. Then this could be:
> 
> ```
> // If a target specific address space was specified, simply return it.
> if (AS >= LangAS::target_first)
>   return AS - LangAS::target_first;
> // For OpenCL, only function local variables are not explicitly marked with
> // an address space in the AST, so treat them as the OpenCL private address 
> space.
> if (!AS && LangOpts.OpenCL)
>   AS = LangAS::opencl_private;
> return (*AddrSpaceMap)[AS];
> ```
> This seems to better express what is happening here. If no address space was 
> specified, and the language is OpenCL, then treat it as OpenCL private and 
> map it according to the target mapping.
> 
> If wanted to eliminate the LangAS::Default named constant then that would be 
> possible as it is no longer being used by name. However, would need to ensure 
> that the first named enumerators starts at 1 so that 0 is left as the "no 
> value explicitly specified" value that each target must map to the target 
> specific generic address space.
I would very much like to see `opencl_private` represented explicitly. This 
would allow us to simplify some parsing and also enable proper support of `NULL 
` literal (that has no AS by the spec).

We can of course do this refactoring work as a separate step.



Comment at: lib/Sema/SemaType.cpp:5537
+llvm::APSInt Offset(addrSpace.getBitWidth(), false);
+Offset = LangAS::target_first;
+addrSpace += Offset;

Do we need this temporary variable here?


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

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

Revised by Tony's comments.


https://reviews.llvm.org/D31404

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  include/clang/Basic/AddressSpaces.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/ExprClassification.cpp
  lib/AST/TypePrinter.cpp
  lib/Basic/Targets.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCL/address-space-constant-initializers.cl
  test/CodeGenOpenCL/address-spaces.cl
  test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
  test/CodeGenOpenCL/vla.cl
  test/Sema/address_spaces.c
  test/Sema/invalid-assignment-constant-address-space.c
  test/SemaOpenCL/invalid-assignment-constant-address-space.cl

Index: test/SemaOpenCL/invalid-assignment-constant-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCL/invalid-assignment-constant-address-space.cl
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+int constant c[3] = {0};
+
+void foo() {
+  c[0] = 1; //expected-error{{read-only variable is not assignable}}
+}
Index: test/Sema/invalid-assignment-constant-address-space.c
===
--- test/Sema/invalid-assignment-constant-address-space.c
+++ /dev/null
@@ -1,8 +0,0 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
-
-#define OPENCL_CONSTANT 8388354
-int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};
-
-void foo() {
-  c[0] = 1; //expected-error{{read-only variable is not assignable}}
-}
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -19,8 +19,8 @@
   _AS1 int array[5];  // expected-error {{automatic variable qualified with an address space}}
   _AS1 int arrarr[5][5]; // expected-error {{automatic variable qualified with an address space}}
 
-  __attribute__((address_space(-1))) int *_boundsA; // expected-error {{address space is negative}}
-  __attribute__((address_space(0x7F))) int *_boundsB;
+  __attribute__((address_space(-1))) int *_boundsA; // expected-warning {{address space is negative}}
+  __attribute__((address_space(0x7F))) int *_boundsB; // expected-error {{address space is larger than the maximum supported}}
   __attribute__((address_space(0x100))) int *_boundsC; // expected-error {{address space is larger than the maximum supported}}
   // chosen specifically to overflow 32 bits and come out reasonable
   __attribute__((address_space(4294967500))) int *_boundsD; // expected-error {{address space is larger than the maximum supported}}
Index: test/CodeGenOpenCL/vla.cl
===
--- test/CodeGenOpenCL/vla.cl
+++ test/CodeGenOpenCL/vla.cl
@@ -1,18 +1,26 @@
-// RUN: %clang_cc1 -emit-llvm -triple "spir-unknown-unknown" -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple "spir-unknown-unknown" -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 -emit-llvm -triple amdgcn-amd-amdhsa-opencl -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 -emit-llvm -triple amdgcn-amd-amdhsa-amdgizcl -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,GIZ %s
 
 constant int sz0 = 5;
-// CHECK: @sz0 = addrspace(2) constant i32 5
+// SPIR: @sz0 = addrspace(2) constant i32 5
+// GIZ: @sz0 = addrspace(4) constant i32 5
 const global int sz1 = 16;
 // CHECK: @sz1 = addrspace(1) constant i32 16
 const constant int sz2 = 8;
-// CHECK: @sz2 = addrspace(2) constant i32 8
+// SPIR: @sz2 = addrspace(2) constant i32 8
+// GIZ: @sz2 = addrspace(4) constant i32 8
 // CHECK: @testvla.vla2 = internal addrspace(3) global [8 x i16] undef
 
 kernel void testvla()
 {
   int vla0[sz0];
-// CHECK: %vla0 = alloca [5 x i32]
+// SPIR: %vla0 = alloca [5 x i32]
+// SPIR-NOT: %vla0 = alloca [5 x i32]{{.*}}addrspace
+// GIZ: %vla0 = alloca [5 x i32]{{.*}}addrspace(5)
   char vla1[sz1];
-// CHECK: %vla1 = alloca [16 x i8]
+// SPIR: %vla1 = alloca [16 x i8]
+// SPIR-NOT: %vla1 = alloca [16 x i8]{{.*}}addrspace
+// GIZ: %vla1 = alloca [16 x i8]{{.*}}addrspace(5)
   local short vla2[sz2];
 }
Index: test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
===
--- test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
+++ test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
@@ -4,6 +4,6 @@
 // RUN: %clang_cc1 %s -O0 -triple amdgcn---amdgizcl -emit-llvm -o - | FileCheck -check-prefix=GIZ %s
 
 // CHECK: target datalayout = "e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
-// GIZ: target datalayout = 

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.



Comment at: include/clang/AST/Type.h:339-340
+auto Addr = getAddressSpace();
+if (Addr == 0)
+  return 0;
+return Addr - LangAS::target_first;

Since you mention this is only used for  `__attribute__((address_space(n)))`, 
why is this check for 0 needed?

If it is needed then to match other places should it simply be:

```
if (Addr)
  return Addr - LangAS::target_first;
return 0;
```



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2451-2453
+def warn_attribute_address_space_negative : Warning<
+  "address space is negative">,
+  InGroup>;

Now the one questionable test has been fixed, should the handling of 
address_space attribute go back to it being an error if n is negative? That 
seems more logical.



Comment at: lib/AST/ASTContext.cpp:9555
+  // alloca.
+  if (AS == LangAS::Default && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();

To be consistent with other places should this simply be:
```
if (!AS && LangOpts.OpenCL)
```



Comment at: lib/AST/ASTContext.cpp:9556
+  if (AS == LangAS::Default && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();
+  if (AS >= LangAS::target_first)

An alternative to doing this would be to add an opencl_private to LangAS and 
have each target map it accordingly. Then this could be:

```
// If a target specific address space was specified, simply return it.
if (AS >= LangAS::target_first)
  return AS - LangAS::target_first;
// For OpenCL, only function local variables are not explicitly marked with
// an address space in the AST, so treat them as the OpenCL private address 
space.
if (!AS && LangOpts.OpenCL)
  AS = LangAS::opencl_private;
return (*AddrSpaceMap)[AS];
```
This seems to better express what is happening here. If no address space was 
specified, and the language is OpenCL, then treat it as OpenCL private and map 
it according to the target mapping.

If wanted to eliminate the LangAS::Default named constant then that would be 
possible as it is no longer being used by name. However, would need to ensure 
that the first named enumerators starts at 1 so that 0 is left as the "no value 
explicitly specified" value that each target must map to the target specific 
generic address space.



Comment at: lib/Sema/SemaExprCXX.cpp:2052
+  else if (AllocType.getQualifiers().getAddressSpace() &&
+  AllocType.getQualifiers().getAddressSpace() != 
LangAS::target_first)
 return Diag(Loc, diag::err_address_space_qualified_new)

Should this be `>= LangAS::target_first` ? Seems it is wanting to check if an 
`__attribute__((address_space(n)))` was specified.



Comment at: lib/Sema/SemaExprCXX.cpp:3123
+if (Pointee.getQualifiers().getAddressSpace() &&
+   Pointee.getQualifiers().getAddressSpace() != LangAS::target_first)
   return Diag(Ex.get()->getLocStart(),

Ditto.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

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

Update for the llvm IRBuilder API change for alloca. Removed data layout 
argument.
Moved a lit test to SemaOpenCL.


https://reviews.llvm.org/D31404

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  include/clang/Basic/AddressSpaces.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/ExprClassification.cpp
  lib/AST/TypePrinter.cpp
  lib/Basic/Targets.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGExpr.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCL/address-space-constant-initializers.cl
  test/CodeGenOpenCL/address-spaces.cl
  test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
  test/CodeGenOpenCL/vla.cl
  test/Sema/address_spaces.c
  test/Sema/invalid-assignment-constant-address-space.c
  test/SemaOpenCL/invalid-assignment-constant-address-space.cl

Index: test/SemaOpenCL/invalid-assignment-constant-address-space.cl
===
--- /dev/null
+++ test/SemaOpenCL/invalid-assignment-constant-address-space.cl
@@ -0,0 +1,7 @@
+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
+
+int constant c[3] = {0};
+
+void foo() {
+  c[0] = 1; //expected-error{{read-only variable is not assignable}}
+}
Index: test/Sema/invalid-assignment-constant-address-space.c
===
--- test/Sema/invalid-assignment-constant-address-space.c
+++ /dev/null
@@ -1,8 +0,0 @@
-// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
-
-#define OPENCL_CONSTANT 8388354
-int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};
-
-void foo() {
-  c[0] = 1; //expected-error{{read-only variable is not assignable}}
-}
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -19,8 +19,8 @@
   _AS1 int array[5];  // expected-error {{automatic variable qualified with an address space}}
   _AS1 int arrarr[5][5]; // expected-error {{automatic variable qualified with an address space}}
 
-  __attribute__((address_space(-1))) int *_boundsA; // expected-error {{address space is negative}}
-  __attribute__((address_space(0x7F))) int *_boundsB;
+  __attribute__((address_space(-1))) int *_boundsA; // expected-warning {{address space is negative}}
+  __attribute__((address_space(0x7F))) int *_boundsB; // expected-error {{address space is larger than the maximum supported}}
   __attribute__((address_space(0x100))) int *_boundsC; // expected-error {{address space is larger than the maximum supported}}
   // chosen specifically to overflow 32 bits and come out reasonable
   __attribute__((address_space(4294967500))) int *_boundsD; // expected-error {{address space is larger than the maximum supported}}
Index: test/CodeGenOpenCL/vla.cl
===
--- test/CodeGenOpenCL/vla.cl
+++ test/CodeGenOpenCL/vla.cl
@@ -1,18 +1,26 @@
-// RUN: %clang_cc1 -emit-llvm -triple "spir-unknown-unknown" -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple "spir-unknown-unknown" -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 -emit-llvm -triple amdgcn-amd-amdhsa-opencl -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 -emit-llvm -triple amdgcn-amd-amdhsa-amdgizcl -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,GIZ %s
 
 constant int sz0 = 5;
-// CHECK: @sz0 = addrspace(2) constant i32 5
+// SPIR: @sz0 = addrspace(2) constant i32 5
+// GIZ: @sz0 = addrspace(4) constant i32 5
 const global int sz1 = 16;
 // CHECK: @sz1 = addrspace(1) constant i32 16
 const constant int sz2 = 8;
-// CHECK: @sz2 = addrspace(2) constant i32 8
+// SPIR: @sz2 = addrspace(2) constant i32 8
+// GIZ: @sz2 = addrspace(4) constant i32 8
 // CHECK: @testvla.vla2 = internal addrspace(3) global [8 x i16] undef
 
 kernel void testvla()
 {
   int vla0[sz0];
-// CHECK: %vla0 = alloca [5 x i32]
+// SPIR: %vla0 = alloca [5 x i32]
+// SPIR-NOT: %vla0 = alloca [5 x i32]{{.*}}addrspace
+// GIZ: %vla0 = alloca [5 x i32]{{.*}}addrspace(5)
   char vla1[sz1];
-// CHECK: %vla1 = alloca [16 x i8]
+// SPIR: %vla1 = alloca [16 x i8]
+// SPIR-NOT: %vla1 = alloca [16 x i8]{{.*}}addrspace
+// GIZ: %vla1 = alloca [16 x i8]{{.*}}addrspace(5)
   local short vla2[sz2];
 }
Index: test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
===
--- test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
+++ test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
@@ -4,6 +4,6 @@
 // RUN: %clang_cc1 %s -O0 -triple amdgcn---amdgizcl -emit-llvm -o - | FileCheck -check-prefix=GIZ %s
 
 // CHECK: target datalayout = 

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

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



Comment at: lib/AST/ASTContext.cpp:9553
+  // alloca.
+  if (AS == LangAS::Default && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();

Anastasia wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > Here it seems that LangAS::Default is indeed opencl_private?
> > For OpenCL, that's true, however LangAS::Default may also be used by other 
> > languages to represent the default address space (i.e. no address space).
> Do we know what the other use cases could be?
It is used in the address space mapping to allow the default address space in 
AST to be mapped to target specified address space for non-OpenCL languages. 
For AMDGPU target, this maps to address space 4 for the non-amdgiz environment.



Comment at: test/Sema/invalid-assignment-constant-address-space.c:4
-#define OPENCL_CONSTANT 8388354
-int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};
 

Anastasia wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > Is this test even correct? I don't think we can assume that C address 
> > > spaces inherit the same restrictions as OpenCL. Especially that the 
> > > notion of private/local/constant/global is an OpenCL specific thing.
> > > 
> > > I feel like Clang doesn't behave correctly for C address spaces now.
> > > 
> > > As for OpenCL I don't see why would anyone use 
> > > __attribute__((address_space())) for constant AS. Especially that it's 
> > > not part of the spec.
> > I agree. There is no guarantee that in C language a user specified address 
> > space which happens to have the same address space value as OpenCL constant 
> > in AST will have the same semantics as OpenCL constant, because we only 
> > guarantee the semantics in OpenCL. For example, if we add a check for 
> > language for this diagnostic, this test will fail.
> > 
> > A user should not expect the same semantics. Only the target address space 
> > in the generated IR is guaranteed.
> I think we should move this tests into CL and use __constant. Also it would 
> be nice to add LangOpts.OpenCL check to where we give the error.
Will do.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In https://reviews.llvm.org/D31404#714906, @yaxunl wrote:

> In https://reviews.llvm.org/D31404#714244, @Anastasia wrote:
>
> > I can't see clearly why the alloca has to be extended to accommodate the 
> > address space too? Couldn't  the address space for alloca just be taken 
> > directly from the data layout?
> >
> > In fact is seems from the LLVM review, an address space for alloca doesn't 
> > go into the bitcode.
>
>
> In the latest comments of the LLVM review, reviewers have agreed that address 
> space of alloca goes into the bitcode.
>
> I am not quite get your first question. Do you mean why the API of alloca has 
> to have an address space parameter? Or do you question the necessity to let 
> alloca returning a pointer pointing to non-zero address space?


I am asking mainly about the motivation of extending alloca instruction syntax 
with an extra address space. It seems to me that the address space for alloca 
could just be taken from the data layout without any modification to the 
instruction itself.




Comment at: include/clang/Basic/AddressSpaces.h:28
 enum ID {
-  Offset = 0x7FFF00,
+  Default = 0,
 

yaxunl wrote:
> Anastasia wrote:
> > Somehow I wish that opencl_private would be represented explicitly instead 
> > and then an absence of an address space attribute would signify the default 
> > one to be used. But since opencl_private has always been represented as an 
> > absence of an address space attribute not only in AST but in IR as well, I 
> > believe it might be a bigger change now. However, how does this default 
> > address space align with default AS we put during type parsing in 
> > processTypeAttrs (https://reviews.llvm.org/D13168). I think after this step 
> > we shouldn't need default AS explicitly any longer? 
> Currently in Clang having an address space qualifier value of 0 is equivalent 
> to having no address space qualifier. This is due to the internal 
> representation of address space qualifier as bits in a mask. Therefore 
> although there are separate API's for hasAddressSpace() and 
> getAddressSpace(), in many many places people just do not use 
> hasAddressSpace() and only use getAddressSpace(). In a way, this is 
> convenient, since it allows people to use just one unsigned to represent that 
> whether a type has an address space qualifier and the value of the qualifier 
> if it has one. That's why value 0 of address space qualifier is called 
> `Default`, since it indicates `no address space qualifier`, which is the 
> default situation. Here we give it the name `Default`, just to emphasise the 
> existing reality, that is, 0 is truely the default value of address space 
> qualifier. This also matches most languages' view of address space, that is, 
> if not explicitly specified, 0 is the default address space qualifier since 
> it means `no address space qualifier`.
> 
> For OpenCL 1.2, this matches perfectly to private address space, since if no 
> address space qualifier implies private address space. For OpenCL 2.0, things 
> become complicated. 'no address space qualifier' in the source code no longer 
> ends up with a fixed address space qualifier in AST. What address space 
> qualifier we get in AST depends on scope of the variable. To be consistent 
> with the AST of OpenCL 1.2, we continue to use 'no address space qualifier 
> (or 0 value address space qualifier)' in AST to represent private address 
> space in OpenCL source language. This is non-ideal but it works fine. 
> Therefore although it is not written, in fact opencl_private is 0.
> 
> Since address space 0 in AST always represents the private address space in 
> OpenCL and the default address space in other languages, it cannot be used 
> for other address spaces of OpenCL. Also, when mapped to target address 
> space, for OpenCL, address space 0 in AST should map to target private 
> address space or alloca address space; for other languages, address space 0 
> in AST should map to target generic address space. It would be clearer to 
> have an enum value for 0 instead of using 0 directly.
I see a little value adding //Default// explicitly here, also because default 
AS is an OpenCL specific thing in my opinion. In C objects are just allowed to 
have no AS and it's fine. In fact the only use of //Default// in your patch is 
synonym to //opencl_private//. Unless we find another use for this, I would 
prefer not to add code for potential future use cases because it adds confusion 
and can be easily misused or create complications for adding new features.

I would rather say that Clang is missing an explicit representation of 
//opencl_private// at the moment. Mainly because private was used as default AS 
before OpenCL 2.0. So it was just the same thing. In OpenCL2.0 we worked around 
the fact that private is not represented explicitly by adding 'missing' ASes in 
//processTypeAttrs//. But it created some issues (for example in 

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In https://reviews.llvm.org/D31404#714244, @Anastasia wrote:

> I can't see clearly why the alloca has to be extended to accommodate the 
> address space too? Couldn't  the address space for alloca just be taken 
> directly from the data layout?
>
> In fact is seems from the LLVM review, an address space for alloca doesn't go 
> into the bitcode.


In the latest comments of the LLVM review, reviewers have agreed that address 
space of alloca goes into the bitcode.

I am not quite get your first question. Do you mean why the API of alloca has 
to have an address space parameter? Or do you question the necessity to let 
alloca returning a pointer pointing to non-zero address space?




Comment at: include/clang/Basic/AddressSpaces.h:28
 enum ID {
-  Offset = 0x7FFF00,
+  Default = 0,
 

Anastasia wrote:
> Somehow I wish that opencl_private would be represented explicitly instead 
> and then an absence of an address space attribute would signify the default 
> one to be used. But since opencl_private has always been represented as an 
> absence of an address space attribute not only in AST but in IR as well, I 
> believe it might be a bigger change now. However, how does this default 
> address space align with default AS we put during type parsing in 
> processTypeAttrs (https://reviews.llvm.org/D13168). I think after this step 
> we shouldn't need default AS explicitly any longer? 
Currently in Clang having an address space qualifier value of 0 is equivalent 
to having no address space qualifier. This is due to the internal 
representation of address space qualifier as bits in a mask. Therefore although 
there are separate API's for hasAddressSpace() and getAddressSpace(), in many 
many places people just do not use hasAddressSpace() and only use 
getAddressSpace(). In a way, this is convenient, since it allows people to use 
just one unsigned to represent that whether a type has an address space 
qualifier and the value of the qualifier if it has one. That's why value 0 of 
address space qualifier is called `Default`, since it indicates `no address 
space qualifier`, which is the default situation. Here we give it the name 
`Default`, just to emphasise the existing reality, that is, 0 is truely the 
default value of address space qualifier. This also matches most languages' 
view of address space, that is, if not explicitly specified, 0 is the default 
address space qualifier since it means `no address space qualifier`.

For OpenCL 1.2, this matches perfectly to private address space, since if no 
address space qualifier implies private address space. For OpenCL 2.0, things 
become complicated. 'no address space qualifier' in the source code no longer 
ends up with a fixed address space qualifier in AST. What address space 
qualifier we get in AST depends on scope of the variable. To be consistent with 
the AST of OpenCL 1.2, we continue to use 'no address space qualifier (or 0 
value address space qualifier)' in AST to represent private address space in 
OpenCL source language. This is non-ideal but it works fine. Therefore although 
it is not written, in fact opencl_private is 0.

Since address space 0 in AST always represents the private address space in 
OpenCL and the default address space in other languages, it cannot be used for 
other address spaces of OpenCL. Also, when mapped to target address space, for 
OpenCL, address space 0 in AST should map to target private address space or 
alloca address space; for other languages, address space 0 in AST should map to 
target generic address space. It would be clearer to have an enum value for 0 
instead of using 0 directly.



Comment at: include/clang/Basic/AddressSpaces.h:41
+
+  target_first = Count
 };

Anastasia wrote:
> I don't entirely understand the motivation for this. I think the idea of 
> LangAS is to represent the source ASes while target ASes are reflected in the 
> Map of Targets.cpp.
There are two types of address spaces in languages end up as address spaces in 
AST:

1. language defined address spaces, e.g. global in OpenCL => mapped to target 
address space
2. `__attribute__((address_space(n)))` => directly used as target address space 
with the same value

Since address space 0 in AST represents the default address space (no address 
space), it must be part of language address spaces and be mapped. Then it may 
be mapped to a target address space which is not 0.

Here is the problem: a user may use `__attribute__((address_space(0)))` to 
specify target address space 0, but he/she cannot, since address space 0 is 
always mapped as a language address space.

To solve this issue, address spaces from `__attribute__((address_space(n)))` is 
added to by Count when stored in AST. When mapped to target address space, 
their value is deducted by Count. Therefore, 
`__attribute__((address_space(0)))` becomes representable in AST.



Comment 

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

I can't see clearly why the alloca has to be extended to accommodate the 
address space too? Couldn't  the address space for alloca just be taken 
directly from the data layout?

In fact is seems from the LLVM review, an address space for alloca doesn't go 
into the bitcode.




Comment at: include/clang/Basic/AddressSpaces.h:28
 enum ID {
-  Offset = 0x7FFF00,
+  Default = 0,
 

Somehow I wish that opencl_private would be represented explicitly instead and 
then an absence of an address space attribute would signify the default one to 
be used. But since opencl_private has always been represented as an absence of 
an address space attribute not only in AST but in IR as well, I believe it 
might be a bigger change now. However, how does this default address space 
align with default AS we put during type parsing in processTypeAttrs 
(https://reviews.llvm.org/D13168). I think after this step we shouldn't need 
default AS explicitly any longer? 



Comment at: include/clang/Basic/AddressSpaces.h:41
+
+  target_first = Count
 };

I don't entirely understand the motivation for this. I think the idea of LangAS 
is to represent the source ASes while target ASes are reflected in the Map of 
Targets.cpp.



Comment at: lib/AST/ASTContext.cpp:9553
+  // alloca.
+  if (AS == LangAS::Default && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();

Here it seems that LangAS::Default is indeed opencl_private?



Comment at: test/Sema/invalid-assignment-constant-address-space.c:4
-#define OPENCL_CONSTANT 8388354
-int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};
 

Is this test even correct? I don't think we can assume that C address spaces 
inherit the same restrictions as OpenCL. Especially that the notion of 
private/local/constant/global is an OpenCL specific thing.

I feel like Clang doesn't behave correctly for C address spaces now.

As for OpenCL I don't see why would anyone use __attribute__((address_space())) 
for constant AS. Especially that it's not part of the spec.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.



Comment at: include/clang/AST/Type.h:336-342
+  /// Get the address space value used in diagnostics.
+  unsigned getAddressSpacePrintValue() const {
+unsigned AS = getAddressSpace();
+if (AS >= LangAS::target_first)
+  return AS - LangAS::target_first;
+return AS;
+  }

yaxunl wrote:
> t-tye wrote:
> > Is this the right thing to do? This is making the CLANG named address 
> > spaces have the same numbers as the target-specific address space numbers 
> > which would seem rather confusing.
> > 
> > What do the diagnostics want to display? For example, they could display 
> > the address space as a human readable form such as "Default", 
> > "OpenCL-global", CUDA-device", "target-specific(5)", etc. To do that this 
> > function could take an iostream and a LangAS value and use << to write to 
> > the iostream.
> This function is used by diagnostics for address spaces specified by 
> `__attribute__((address_space(n)))`. There are several lit tests for that, 
> e.g. 
> 
> https://github.com/llvm-mirror/clang/blob/master/test/SemaCXX/address-space-newdelete.cpp
> https://github.com/llvm-mirror/clang/blob/master/test/SemaCXX/address-space-references.cpp
> 
> It is desirable to use the same value as specified by the attribute, 
> otherwise it may confuse the user.
I can change it to

```
  unsigned getAddressSpacePrintValue() const {
return getAddressSpace() - LangAS::target_first;
  }
```
Since the value is only used for `__attribute__((address_space(n)))`, in case 
the user specifies negative value to achieve language specific addr space, the 
diag msg will just show the same negative value they used in  
`__attribute__((address_space(n)))`


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked an inline comment as done.
yaxunl added inline comments.



Comment at: include/clang/AST/ASTContext.h:2319
 return AddrSpaceMapMangling || 
-   AS < LangAS::Offset || 
-   AS >= LangAS::Offset + LangAS::Count;
+   AS > LangAS::target_first;
   }

t-tye wrote:
> Should this be >= since it wants to return for all target address spaces, and 
> target_first is the first one?
Will do.



Comment at: include/clang/AST/Type.h:336-342
+  /// Get the address space value used in diagnostics.
+  unsigned getAddressSpacePrintValue() const {
+unsigned AS = getAddressSpace();
+if (AS >= LangAS::target_first)
+  return AS - LangAS::target_first;
+return AS;
+  }

t-tye wrote:
> Is this the right thing to do? This is making the CLANG named address spaces 
> have the same numbers as the target-specific address space numbers which 
> would seem rather confusing.
> 
> What do the diagnostics want to display? For example, they could display the 
> address space as a human readable form such as "Default", "OpenCL-global", 
> CUDA-device", "target-specific(5)", etc. To do that this function could take 
> an iostream and a LangAS value and use << to write to the iostream.
This function is used by diagnostics for address spaces specified by 
`__attribute__((address_space(n)))`. There are several lit tests for that, e.g. 

https://github.com/llvm-mirror/clang/blob/master/test/SemaCXX/address-space-newdelete.cpp
https://github.com/llvm-mirror/clang/blob/master/test/SemaCXX/address-space-references.cpp

It is desirable to use the same value as specified by the attribute, otherwise 
it may confuse the user.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.



Comment at: include/clang/AST/ASTContext.h:2319
 return AddrSpaceMapMangling || 
-   AS < LangAS::Offset || 
-   AS >= LangAS::Offset + LangAS::Count;
+   AS > LangAS::target_first;
   }

Should this be >= since it wants to return for all target address spaces, and 
target_first is the first one?



Comment at: include/clang/AST/Type.h:336-342
+  /// Get the address space value used in diagnostics.
+  unsigned getAddressSpacePrintValue() const {
+unsigned AS = getAddressSpace();
+if (AS >= LangAS::target_first)
+  return AS - LangAS::target_first;
+return AS;
+  }

Is this the right thing to do? This is making the CLANG named address spaces 
have the same numbers as the target-specific address space numbers which would 
seem rather confusing.

What do the diagnostics want to display? For example, they could display the 
address space as a human readable form such as "Default", "OpenCL-global", 
CUDA-device", "target-specific(5)", etc. To do that this function could take an 
iostream and a LangAS value and use << to write to the iostream.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

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

Revised by Tony's comments.


https://reviews.llvm.org/D31404

Files:
  include/clang/AST/ASTContext.h
  include/clang/AST/Type.h
  include/clang/Basic/AddressSpaces.h
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/AST/ASTContext.cpp
  lib/AST/TypePrinter.cpp
  lib/Basic/Targets.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGObjCGNU.cpp
  lib/Sema/SemaExprCXX.cpp
  lib/Sema/SemaOverload.cpp
  lib/Sema/SemaType.cpp
  test/CodeGenOpenCL/address-space-constant-initializers.cl
  test/CodeGenOpenCL/address-spaces.cl
  test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
  test/CodeGenOpenCL/vla.cl
  test/Sema/address_spaces.c
  test/Sema/invalid-assignment-constant-address-space.c

Index: test/Sema/invalid-assignment-constant-address-space.c
===
--- test/Sema/invalid-assignment-constant-address-space.c
+++ test/Sema/invalid-assignment-constant-address-space.c
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
 
-#define OPENCL_CONSTANT 8388354
-int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};
+#define OPENCL_CONSTANT -5
+int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0}; //expected-warning{{address space is negative}}
 
 void foo() {
   c[0] = 1; //expected-error{{read-only variable is not assignable}}
Index: test/Sema/address_spaces.c
===
--- test/Sema/address_spaces.c
+++ test/Sema/address_spaces.c
@@ -19,8 +19,8 @@
   _AS1 int array[5];  // expected-error {{automatic variable qualified with an address space}}
   _AS1 int arrarr[5][5]; // expected-error {{automatic variable qualified with an address space}}
 
-  __attribute__((address_space(-1))) int *_boundsA; // expected-error {{address space is negative}}
-  __attribute__((address_space(0x7F))) int *_boundsB;
+  __attribute__((address_space(-1))) int *_boundsA; // expected-warning {{address space is negative}}
+  __attribute__((address_space(0x7F))) int *_boundsB; // expected-error {{address space is larger than the maximum supported}}
   __attribute__((address_space(0x100))) int *_boundsC; // expected-error {{address space is larger than the maximum supported}}
   // chosen specifically to overflow 32 bits and come out reasonable
   __attribute__((address_space(4294967500))) int *_boundsD; // expected-error {{address space is larger than the maximum supported}}
Index: test/CodeGenOpenCL/vla.cl
===
--- test/CodeGenOpenCL/vla.cl
+++ test/CodeGenOpenCL/vla.cl
@@ -1,18 +1,26 @@
-// RUN: %clang_cc1 -emit-llvm -triple "spir-unknown-unknown" -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple "spir-unknown-unknown" -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 -emit-llvm -triple amdgcn-amd-amdhsa-opencl -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 -emit-llvm -triple amdgcn-amd-amdhsa-amdgizcl -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,GIZ %s
 
 constant int sz0 = 5;
-// CHECK: @sz0 = addrspace(2) constant i32 5
+// SPIR: @sz0 = addrspace(2) constant i32 5
+// GIZ: @sz0 = addrspace(4) constant i32 5
 const global int sz1 = 16;
 // CHECK: @sz1 = addrspace(1) constant i32 16
 const constant int sz2 = 8;
-// CHECK: @sz2 = addrspace(2) constant i32 8
+// SPIR: @sz2 = addrspace(2) constant i32 8
+// GIZ: @sz2 = addrspace(4) constant i32 8
 // CHECK: @testvla.vla2 = internal addrspace(3) global [8 x i16] undef
 
 kernel void testvla()
 {
   int vla0[sz0];
-// CHECK: %vla0 = alloca [5 x i32]
+// SPIR: %vla0 = alloca [5 x i32]
+// SPIR-NOT: %vla0 = alloca [5 x i32]{{.*}}addrspace
+// GIZ: %vla0 = alloca [5 x i32]{{.*}}addrspace(5)
   char vla1[sz1];
-// CHECK: %vla1 = alloca [16 x i8]
+// SPIR: %vla1 = alloca [16 x i8]
+// SPIR-NOT: %vla1 = alloca [16 x i8]{{.*}}addrspace
+// GIZ: %vla1 = alloca [16 x i8]{{.*}}addrspace(5)
   local short vla2[sz2];
 }
Index: test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
===
--- test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
+++ test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
@@ -4,6 +4,6 @@
 // RUN: %clang_cc1 %s -O0 -triple amdgcn---amdgizcl -emit-llvm -o - | FileCheck -check-prefix=GIZ %s
 
 // CHECK: target datalayout = "e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
-// GIZ: target datalayout = "e-p:64:64-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
+// GIZ: target datalayout = 

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.



Comment at: lib/AST/ASTContext.cpp:9547-9555
+unsigned ASTContext::getTargetAddressSpace(unsigned AS) const {
+  // For OpenCL, the address space qualifier is 0 in AST.
+  if (AS == 0 && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();
+  if (AS < LangAS::Offset || AS >= LangAS::Offset + LangAS::Count)
+return AS;
+  else

yaxunl wrote:
> t-tye wrote:
> > yaxunl wrote:
> > > t-tye wrote:
> > > > Presumably this will not work if the user put an address space 
> > > > attribute on a variable with an address space of 0, since it is not 
> > > > possible to distinguish between a variable that never had an attribute 
> > > > and was function local, and one that has an explicit address space 
> > > > attribute specifying 0.
> > > > 
> > > > It would seem better if LangAS did not map the 0..LangAS::Offset to be 
> > > > target address spaces. Instead LangAS could use 0..LangAS::Count to be 
> > > > the CLANG explicitly specified values (reserving 0 to mean the default 
> > > > when none was specified), and values above LangAS::Count would map to 
> > > > the explicitly specified target address spaces. For example:
> > > > 
> > > > ```
> > > > namespace clang {
> > > >  
> > > >  namespace LangAS {
> > > >  
> > > >  /// \brief Defines the set of possible language-specific address 
> > > > spaces.
> > > >  ///
> > > >  /// This uses values above the language-specific address spaces to 
> > > > denote
> > > >  /// the target-specific address spaces biased by target_first.
> > > >  enum ID {
> > > >default = 0,
> > > >  
> > > >opencl_global,
> > > >opencl_local,
> > > >opencl_constant,
> > > >opencl_generic,
> > > >  
> > > >cuda_device,
> > > >cuda_constant,
> > > >cuda_shared,
> > > >  
> > > >Count,
> > > > 
> > > >target_first = Count
> > > >  };
> > > >  
> > > >  /// The type of a lookup table which maps from language-specific 
> > > > address spaces
> > > >  /// to target-specific ones.
> > > >  typedef unsigned Map[Count];
> > > >  
> > > >  }
> > > > ```
> > > > 
> > > > Then this function would be:
> > > > 
> > > > ```
> > > > unsigned ASTContext::getTargetAddressSpace(unsigned AS) const {
> > > >   if (AS == LangAS::default && LangOpts.OpenCL)
> > > > // For OpenCL, only function local variables are not explicitly 
> > > > marked with an
> > > > // address space in the AST, and these need to be the address space 
> > > > of alloca.
> > > > return getTargetInfo().getDataLayout().getAllocaAddrSpace();
> > > >   if (AS >= LangAS::target_first)
> > > > return AS - LangAS::target_first;
> > > >   else
> > > > return (*AddrSpaceMap)[AS];
> > > > }
> > > > ```
> > > > 
> > > > Each target AddrSpaceMap would map LangAS::default to that target's 
> > > > default generic address space since that matches most other languages.
> > > > 
> > > > The address space attribute would need a corresponding "+ 
> > > > LangAS::target_first" to the value it stored in the AST.
> > > > 
> > > > Then it is possible to definitively tell when an AST node has not had 
> > > > any address space specified as it will be the LangAS::default value.
> > > There is a lit test like this:
> > > 
> > > ```
> > > // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
> > > 
> > > #define OPENCL_CONSTANT 8388354
> > > int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};
> > > 
> > > void foo() {
> > >   c[0] = 1; //expected-error{{read-only variable is not assignable}}
> > > }
> > > 
> > > ```
> > > It tries to set address space of opencl_constant through 
> > > `__attribute__((address_space(n)))`. If we "+ LangAS::target_first" to 
> > > the value before it is tored in the AST, we are not able to use 
> > > `__attribute__((address_space(n)))` to represent opencl_constant.
> > This seems a bit of a hack. It could be made to work by simply defining 
> > OPENCL_CONSTANT to be the negative value that would result in the correct 
> > LangAS value, which is pretty much what the test is doing anyway. Just 
> > seems conflating the default value with the first target address space 
> > value is undesirable as it prevents specifying target address space 0 as 
> > that gets treated differently than any other address space value.
> Clang will emit error if address space value is negative, but I can change it 
> to a warning.
I guess the observation is that without a change the proposed changes will make 
__attribute__((address_space(0))) behave in unexpected ways for some targets 
(for AMDGPU it would actually cause address space for private to be used). The 
suggested approach also seems cleaner as it explicitly defines a "default" 
value which does not overlay the target address space range of values.


https://reviews.llvm.org/D31404



___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: lib/AST/ASTContext.cpp:9547-9555
+unsigned ASTContext::getTargetAddressSpace(unsigned AS) const {
+  // For OpenCL, the address space qualifier is 0 in AST.
+  if (AS == 0 && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();
+  if (AS < LangAS::Offset || AS >= LangAS::Offset + LangAS::Count)
+return AS;
+  else

t-tye wrote:
> yaxunl wrote:
> > t-tye wrote:
> > > Presumably this will not work if the user put an address space attribute 
> > > on a variable with an address space of 0, since it is not possible to 
> > > distinguish between a variable that never had an attribute and was 
> > > function local, and one that has an explicit address space attribute 
> > > specifying 0.
> > > 
> > > It would seem better if LangAS did not map the 0..LangAS::Offset to be 
> > > target address spaces. Instead LangAS could use 0..LangAS::Count to be 
> > > the CLANG explicitly specified values (reserving 0 to mean the default 
> > > when none was specified), and values above LangAS::Count would map to the 
> > > explicitly specified target address spaces. For example:
> > > 
> > > ```
> > > namespace clang {
> > >  
> > >  namespace LangAS {
> > >  
> > >  /// \brief Defines the set of possible language-specific address spaces.
> > >  ///
> > >  /// This uses values above the language-specific address spaces to denote
> > >  /// the target-specific address spaces biased by target_first.
> > >  enum ID {
> > >default = 0,
> > >  
> > >opencl_global,
> > >opencl_local,
> > >opencl_constant,
> > >opencl_generic,
> > >  
> > >cuda_device,
> > >cuda_constant,
> > >cuda_shared,
> > >  
> > >Count,
> > > 
> > >target_first = Count
> > >  };
> > >  
> > >  /// The type of a lookup table which maps from language-specific address 
> > > spaces
> > >  /// to target-specific ones.
> > >  typedef unsigned Map[Count];
> > >  
> > >  }
> > > ```
> > > 
> > > Then this function would be:
> > > 
> > > ```
> > > unsigned ASTContext::getTargetAddressSpace(unsigned AS) const {
> > >   if (AS == LangAS::default && LangOpts.OpenCL)
> > > // For OpenCL, only function local variables are not explicitly 
> > > marked with an
> > > // address space in the AST, and these need to be the address space 
> > > of alloca.
> > > return getTargetInfo().getDataLayout().getAllocaAddrSpace();
> > >   if (AS >= LangAS::target_first)
> > > return AS - LangAS::target_first;
> > >   else
> > > return (*AddrSpaceMap)[AS];
> > > }
> > > ```
> > > 
> > > Each target AddrSpaceMap would map LangAS::default to that target's 
> > > default generic address space since that matches most other languages.
> > > 
> > > The address space attribute would need a corresponding "+ 
> > > LangAS::target_first" to the value it stored in the AST.
> > > 
> > > Then it is possible to definitively tell when an AST node has not had any 
> > > address space specified as it will be the LangAS::default value.
> > There is a lit test like this:
> > 
> > ```
> > // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
> > 
> > #define OPENCL_CONSTANT 8388354
> > int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};
> > 
> > void foo() {
> >   c[0] = 1; //expected-error{{read-only variable is not assignable}}
> > }
> > 
> > ```
> > It tries to set address space of opencl_constant through 
> > `__attribute__((address_space(n)))`. If we "+ LangAS::target_first" to the 
> > value before it is tored in the AST, we are not able to use 
> > `__attribute__((address_space(n)))` to represent opencl_constant.
> This seems a bit of a hack. It could be made to work by simply defining 
> OPENCL_CONSTANT to be the negative value that would result in the correct 
> LangAS value, which is pretty much what the test is doing anyway. Just seems 
> conflating the default value with the first target address space value is 
> undesirable as it prevents specifying target address space 0 as that gets 
> treated differently than any other address space value.
Clang will emit error if address space value is negative, but I can change it 
to a warning.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.



Comment at: lib/AST/ASTContext.cpp:9547-9555
+unsigned ASTContext::getTargetAddressSpace(unsigned AS) const {
+  // For OpenCL, the address space qualifier is 0 in AST.
+  if (AS == 0 && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();
+  if (AS < LangAS::Offset || AS >= LangAS::Offset + LangAS::Count)
+return AS;
+  else

yaxunl wrote:
> t-tye wrote:
> > Presumably this will not work if the user put an address space attribute on 
> > a variable with an address space of 0, since it is not possible to 
> > distinguish between a variable that never had an attribute and was function 
> > local, and one that has an explicit address space attribute specifying 0.
> > 
> > It would seem better if LangAS did not map the 0..LangAS::Offset to be 
> > target address spaces. Instead LangAS could use 0..LangAS::Count to be the 
> > CLANG explicitly specified values (reserving 0 to mean the default when 
> > none was specified), and values above LangAS::Count would map to the 
> > explicitly specified target address spaces. For example:
> > 
> > ```
> > namespace clang {
> >  
> >  namespace LangAS {
> >  
> >  /// \brief Defines the set of possible language-specific address spaces.
> >  ///
> >  /// This uses values above the language-specific address spaces to denote
> >  /// the target-specific address spaces biased by target_first.
> >  enum ID {
> >default = 0,
> >  
> >opencl_global,
> >opencl_local,
> >opencl_constant,
> >opencl_generic,
> >  
> >cuda_device,
> >cuda_constant,
> >cuda_shared,
> >  
> >Count,
> > 
> >target_first = Count
> >  };
> >  
> >  /// The type of a lookup table which maps from language-specific address 
> > spaces
> >  /// to target-specific ones.
> >  typedef unsigned Map[Count];
> >  
> >  }
> > ```
> > 
> > Then this function would be:
> > 
> > ```
> > unsigned ASTContext::getTargetAddressSpace(unsigned AS) const {
> >   if (AS == LangAS::default && LangOpts.OpenCL)
> > // For OpenCL, only function local variables are not explicitly marked 
> > with an
> > // address space in the AST, and these need to be the address space of 
> > alloca.
> > return getTargetInfo().getDataLayout().getAllocaAddrSpace();
> >   if (AS >= LangAS::target_first)
> > return AS - LangAS::target_first;
> >   else
> > return (*AddrSpaceMap)[AS];
> > }
> > ```
> > 
> > Each target AddrSpaceMap would map LangAS::default to that target's default 
> > generic address space since that matches most other languages.
> > 
> > The address space attribute would need a corresponding "+ 
> > LangAS::target_first" to the value it stored in the AST.
> > 
> > Then it is possible to definitively tell when an AST node has not had any 
> > address space specified as it will be the LangAS::default value.
> There is a lit test like this:
> 
> ```
> // RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only
> 
> #define OPENCL_CONSTANT 8388354
> int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};
> 
> void foo() {
>   c[0] = 1; //expected-error{{read-only variable is not assignable}}
> }
> 
> ```
> It tries to set address space of opencl_constant through 
> `__attribute__((address_space(n)))`. If we "+ LangAS::target_first" to the 
> value before it is tored in the AST, we are not able to use 
> `__attribute__((address_space(n)))` to represent opencl_constant.
This seems a bit of a hack. It could be made to work by simply defining 
OPENCL_CONSTANT to be the negative value that would result in the correct 
LangAS value, which is pretty much what the test is doing anyway. Just seems 
conflating the default value with the first target address space value is 
undesirable as it prevents specifying target address space 0 as that gets 
treated differently than any other address space value.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added inline comments.



Comment at: lib/AST/ASTContext.cpp:9547-9555
+unsigned ASTContext::getTargetAddressSpace(unsigned AS) const {
+  // For OpenCL, the address space qualifier is 0 in AST.
+  if (AS == 0 && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();
+  if (AS < LangAS::Offset || AS >= LangAS::Offset + LangAS::Count)
+return AS;
+  else

t-tye wrote:
> Presumably this will not work if the user put an address space attribute on a 
> variable with an address space of 0, since it is not possible to distinguish 
> between a variable that never had an attribute and was function local, and 
> one that has an explicit address space attribute specifying 0.
> 
> It would seem better if LangAS did not map the 0..LangAS::Offset to be target 
> address spaces. Instead LangAS could use 0..LangAS::Count to be the CLANG 
> explicitly specified values (reserving 0 to mean the default when none was 
> specified), and values above LangAS::Count would map to the explicitly 
> specified target address spaces. For example:
> 
> ```
> namespace clang {
>  
>  namespace LangAS {
>  
>  /// \brief Defines the set of possible language-specific address spaces.
>  ///
>  /// This uses values above the language-specific address spaces to denote
>  /// the target-specific address spaces biased by target_first.
>  enum ID {
>default = 0,
>  
>opencl_global,
>opencl_local,
>opencl_constant,
>opencl_generic,
>  
>cuda_device,
>cuda_constant,
>cuda_shared,
>  
>Count,
> 
>target_first = Count
>  };
>  
>  /// The type of a lookup table which maps from language-specific address 
> spaces
>  /// to target-specific ones.
>  typedef unsigned Map[Count];
>  
>  }
> ```
> 
> Then this function would be:
> 
> ```
> unsigned ASTContext::getTargetAddressSpace(unsigned AS) const {
>   if (AS == LangAS::default && LangOpts.OpenCL)
> // For OpenCL, only function local variables are not explicitly marked 
> with an
> // address space in the AST, and these need to be the address space of 
> alloca.
> return getTargetInfo().getDataLayout().getAllocaAddrSpace();
>   if (AS >= LangAS::target_first)
> return AS - LangAS::target_first;
>   else
> return (*AddrSpaceMap)[AS];
> }
> ```
> 
> Each target AddrSpaceMap would map LangAS::default to that target's default 
> generic address space since that matches most other languages.
> 
> The address space attribute would need a corresponding "+ 
> LangAS::target_first" to the value it stored in the AST.
> 
> Then it is possible to definitively tell when an AST node has not had any 
> address space specified as it will be the LangAS::default value.
There is a lit test like this:

```
// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only

#define OPENCL_CONSTANT 8388354
int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};

void foo() {
  c[0] = 1; //expected-error{{read-only variable is not assignable}}
}

```
It tries to set address space of opencl_constant through 
`__attribute__((address_space(n)))`. If we "+ LangAS::target_first" to the 
value before it is tored in the AST, we are not able to use 
`__attribute__((address_space(n)))` to represent opencl_constant.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Tony Tye via Phabricator via cfe-commits]

t-tye added inline comments.



Comment at: lib/AST/ASTContext.cpp:9547-9555
+unsigned ASTContext::getTargetAddressSpace(unsigned AS) const {
+  // For OpenCL, the address space qualifier is 0 in AST.
+  if (AS == 0 && LangOpts.OpenCL)
+return getTargetInfo().getDataLayout().getAllocaAddrSpace();
+  if (AS < LangAS::Offset || AS >= LangAS::Offset + LangAS::Count)
+return AS;
+  else

Presumably this will not work if the user put an address space attribute on a 
variable with an address space of 0, since it is not possible to distinguish 
between a variable that never had an attribute and was function local, and one 
that has an explicit address space attribute specifying 0.

It would seem better if LangAS did not map the 0..LangAS::Offset to be target 
address spaces. Instead LangAS could use 0..LangAS::Count to be the CLANG 
explicitly specified values (reserving 0 to mean the default when none was 
specified), and values above LangAS::Count would map to the explicitly 
specified target address spaces. For example:

```
namespace clang {
 
 namespace LangAS {
 
 /// \brief Defines the set of possible language-specific address spaces.
 ///
 /// This uses values above the language-specific address spaces to denote
 /// the target-specific address spaces biased by target_first.
 enum ID {
   default = 0,
 
   opencl_global,
   opencl_local,
   opencl_constant,
   opencl_generic,
 
   cuda_device,
   cuda_constant,
   cuda_shared,
 
   Count,

   target_first = Count
 };
 
 /// The type of a lookup table which maps from language-specific address spaces
 /// to target-specific ones.
 typedef unsigned Map[Count];
 
 }
```

Then this function would be:

```
unsigned ASTContext::getTargetAddressSpace(unsigned AS) const {
  if (AS == LangAS::default && LangOpts.OpenCL)
// For OpenCL, only function local variables are not explicitly marked with 
an
// address space in the AST, and these need to be the address space of 
alloca.
return getTargetInfo().getDataLayout().getAllocaAddrSpace();
  if (AS >= LangAS::target_first)
return AS - LangAS::target_first;
  else
return (*AddrSpaceMap)[AS];
}
```

Each target AddrSpaceMap would map LangAS::default to that target's default 
generic address space since that matches most other languages.

The address space attribute would need a corresponding "+ LangAS::target_first" 
to the value it stored in the AST.

Then it is possible to definitively tell when an AST node has not had any 
address space specified as it will be the LangAS::default value.


https://reviews.llvm.org/D31404



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

[PATCH] D31404: [OpenCL] Allow alloca return non-zero private pointer

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl created this revision.
Herald added subscribers: nhaehnle, wdng.

There is an incoming change in LLVM allowing alloca to return a private pointer 
which does not pointing to address space 0:

https://reviews.llvm.org/D31042#03b9d490

After this change is committed, alloca will return a pointer pointing to an 
address space specified by the data layout (so called alloca addr space, which 
is the last component of the data layout, e.g. A5 indicating alloca address 
space is 5). A data layout not specifying alloca address space will assume it 
is 0, therefore keeping the original behaviour.

Clang codegen needs to make corresponding changes to account for the API change 
of alloca. The change is straightforward. Basically when creating alloca, use 
the alloca address space specified by the data layout.

For OpenCL, the private address space qualifier is 0 in AST. Before this 
change, 0 address space qualifier is always mapped to target address space 0. 
As now target private address space is specified by alloca address space in 
data layout, address space qualifier 0 needs to be mapped to alloca addr space 
specified by the data layout.

This change has no impact on targets whose alloca addr space is 0.


https://reviews.llvm.org/D31404

Files:
  include/clang/AST/ASTContext.h
  lib/AST/ASTContext.cpp
  lib/Basic/Targets.cpp
  lib/CodeGen/CGBuiltin.cpp
  lib/CodeGen/CGCall.cpp
  lib/CodeGen/CGDecl.cpp
  lib/CodeGen/CGExpr.cpp
  lib/CodeGen/CGObjCGNU.cpp
  test/CodeGenOpenCL/address-spaces.cl
  test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
  test/CodeGenOpenCL/vla.cl

Index: test/CodeGenOpenCL/vla.cl
===
--- test/CodeGenOpenCL/vla.cl
+++ test/CodeGenOpenCL/vla.cl
@@ -1,18 +1,26 @@
-// RUN: %clang_cc1 -emit-llvm -triple "spir-unknown-unknown" -O0 -cl-std=CL2.0 -o - %s | FileCheck %s
+// RUN: %clang_cc1 -emit-llvm -triple "spir-unknown-unknown" -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 -emit-llvm -triple amdgcn-amd-amdhsa-opencl -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 -emit-llvm -triple amdgcn-amd-amdhsa-amdgizcl -O0 -cl-std=CL2.0 -o - %s | FileCheck -check-prefixes=CHECK,GIZ %s
 
 constant int sz0 = 5;
-// CHECK: @sz0 = addrspace(2) constant i32 5
+// SPIR: @sz0 = addrspace(2) constant i32 5
+// GIZ: @sz0 = addrspace(4) constant i32 5
 const global int sz1 = 16;
 // CHECK: @sz1 = addrspace(1) constant i32 16
 const constant int sz2 = 8;
-// CHECK: @sz2 = addrspace(2) constant i32 8
+// SPIR: @sz2 = addrspace(2) constant i32 8
+// GIZ: @sz2 = addrspace(4) constant i32 8
 // CHECK: @testvla.vla2 = internal addrspace(3) global [8 x i16] undef
 
 kernel void testvla()
 {
   int vla0[sz0];
-// CHECK: %vla0 = alloca [5 x i32]
+// SPIR: %vla0 = alloca [5 x i32]
+// SPIR-NOT: %vla0 = alloca [5 x i32]{{.*}}addrspace
+// GIZ: %vla0 = alloca [5 x i32]{{.*}}addrspace(5)
   char vla1[sz1];
-// CHECK: %vla1 = alloca [16 x i8]
+// SPIR: %vla1 = alloca [16 x i8]
+// SPIR-NOT: %vla1 = alloca [16 x i8]{{.*}}addrspace
+// GIZ: %vla1 = alloca [16 x i8]{{.*}}addrspace(5)
   local short vla2[sz2];
 }
Index: test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
===
--- test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
+++ test/CodeGenOpenCL/amdgpu-env-amdgiz.cl
@@ -4,6 +4,6 @@
 // RUN: %clang_cc1 %s -O0 -triple amdgcn---amdgizcl -emit-llvm -o - | FileCheck -check-prefix=GIZ %s
 
 // CHECK: target datalayout = "e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
-// GIZ: target datalayout = "e-p:64:64-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
+// GIZ: target datalayout = "e-p:64:64-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64-A5"
 void foo(void) {}
 
Index: test/CodeGenOpenCL/address-spaces.cl
===
--- test/CodeGenOpenCL/address-spaces.cl
+++ test/CodeGenOpenCL/address-spaces.cl
@@ -1,44 +1,57 @@
-// RUN: %clang_cc1 %s -O0 -ffake-address-space-map -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 %s -O0 -DCL20 -cl-std=CL2.0 -ffake-address-space-map -emit-llvm -o - | FileCheck %s --check-prefix=CL20
-
-// CHECK: i32* %arg
+// RUN: %clang_cc1 %s -O0 -ffake-address-space-map -emit-llvm -o - | FileCheck %s --check-prefixes=CHECK,SPIR
+// RUN: %clang_cc1 %s -O0 -DCL20 -cl-std=CL2.0 -ffake-address-space-map -emit-llvm -o - | FileCheck %s --check-prefixes=CL20,CL20SPIR
+// RUN: %clang_cc1 %s -O0 -triple amdgcn-amd-amdhsa-opencl -emit-llvm -o - | FileCheck --check-prefixes=CHECK,SPIR %s
+// RUN: %clang_cc1 %s -O0 -triple amdgcn-amd-amdhsa-opencl -DCL20 -cl-std=CL2.0 -emit-llvm -o - |