[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<DiagGroup<"addr-space">>;
----------------
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