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

Anastasia Stulova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Apr 6 10:05:56 PDT 2017


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 (Pointee.getQualifiers().getAddressSpace())
       return Diag(Ex.get()->getLocStart(),
----------------
yaxunl wrote:
> Anastasia wrote:
> > `Pointee.getAddressSpace()` wouldn't work any more?
> It still works. I will fix it.
Do you plan to update this?


================
Comment at: test/CodeGen/default-address-space.c:51
+// CHECK-LABEL: define void @test4(i32* %a)
+// CHECK: %[[a_addr:.*]] = alloca i32*, align 4, addrspace(5)
+// CHECK: store i32* %a, i32* addrspace(5)* %[[a_addr]]
----------------
The default AS is not the same as alloca AS then?


https://reviews.llvm.org/D31404





More information about the cfe-commits mailing list