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

Anastasia Stulova via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Mon Apr 3 08:38:01 PDT 2017


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





More information about the cfe-commits mailing list