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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl marked 3 inline comments as done.
yaxunl added inline comments.


================
Comment at: lib/AST/ASTContext.cpp:9553
+  // alloca.
+  if (AS == LangAS::Default && LangOpts.OpenCL)
+    return getTargetInfo().getDataLayout().getAllocaAddrSpace();
----------------
Anastasia wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > Here it seems that LangAS::Default is indeed opencl_private?
> > For OpenCL, that's true, however LangAS::Default may also be used by other languages to represent the default address space (i.e. no address space).
> Do we know what the other use cases could be?
It is used in the address space mapping to allow the default address space in AST to be mapped to target specified address space for non-OpenCL languages. For AMDGPU target, this maps to address space 4 for the non-amdgiz environment.


================
Comment at: test/Sema/invalid-assignment-constant-address-space.c:4
-#define OPENCL_CONSTANT 8388354
-int __attribute__((address_space(OPENCL_CONSTANT))) c[3] = {0};
 
----------------
Anastasia wrote:
> yaxunl wrote:
> > Anastasia wrote:
> > > 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.
> > I agree. There is no guarantee that in C language a user specified address space which happens to have the same address space value as OpenCL constant in AST will have the same semantics as OpenCL constant, because we only guarantee the semantics in OpenCL. For example, if we add a check for language for this diagnostic, this test will fail.
> > 
> > A user should not expect the same semantics. Only the target address space in the generated IR is guaranteed.
> I think we should move this tests into CL and use __constant. Also it would be nice to add LangOpts.OpenCL check to where we give the error.
Will do.


https://reviews.llvm.org/D31404