[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<DiagGroup<"addr-space">>;
----------------
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) {
----------------
t-tye wrote:
> Would suggest renaming getAddressSpacePrintValue to getAddressSpaceAttributePrintValue since it only deals with address spaces coming from the` __attribute__((address_space(n)))`.
Will do


================
Comment at: lib/Sema/SemaType.cpp:5537
+    llvm::APSInt Offset(addrSpace.getBitWidth(), false);
+    Offset = LangAS::target_first;
+    addrSpace += Offset;
----------------
Anastasia wrote:
> Do we need this temporary variable here?
will remove it.


https://reviews.llvm.org/D31404