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

Anastasia Stulova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Apr 4 10:30:52 PDT 2017


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





More information about the cfe-commits mailing list