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

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

I can't see clearly why the alloca has to be extended to accommodate the address space too? Couldn't  the address space for alloca just be taken directly from the data layout?

In fact is seems from the LLVM review, an address space for alloca doesn't go into the bitcode.



================
Comment at: include/clang/Basic/AddressSpaces.h:28
 enum ID {
-  Offset = 0x7FFF00,
+  Default = 0,
 
----------------
Somehow I wish that opencl_private would be represented explicitly instead and then an absence of an address space attribute would signify the default one to be used. But since opencl_private has always been represented as an absence of an address space attribute not only in AST but in IR as well, I believe it might be a bigger change now. However, how does this default address space align with default AS we put during type parsing in processTypeAttrs (https://reviews.llvm.org/D13168). I think after this step we shouldn't need default AS explicitly any longer? 


================
Comment at: include/clang/Basic/AddressSpaces.h:41
+
+  target_first = Count
 };
----------------
I don't entirely understand the motivation for this. I think the idea of LangAS is to represent the source ASes while target ASes are reflected in the Map of Targets.cpp.


================
Comment at: lib/AST/ASTContext.cpp:9553
+  // alloca.
+  if (AS == LangAS::Default && LangOpts.OpenCL)
+    return getTargetInfo().getDataLayout().getAllocaAddrSpace();
----------------
Here it seems that LangAS::Default is indeed opencl_private?


================
Comment at: test/Sema/invalid-assignment-constant-address-space.c:4
-#define OPENCL_CONSTANT 8388354
-int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};
 
----------------
Is this test even correct? I don't think we can assume that C address spaces inherit the same restrictions as OpenCL. Especially that the notion of private/local/constant/global is an OpenCL specific thing.

I feel like Clang doesn't behave correctly for C address spaces now.

As for OpenCL I don't see why would anyone use __attribute__((address_space())) for constant AS. Especially that it's not part of the spec.


https://reviews.llvm.org/D31404