[PATCH] D35082: [OpenCL] Add LangAS::opencl_private to represent private address space in AST

Yaxun Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Jul 13 11:59:54 PDT 2017


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


================
Comment at: lib/AST/Expr.cpp:3235
+        // has non-default address space it is not treated as nullptr.
+        bool PointeeHasDefaultAS = false;
+        if (Ctx.getLangOpts().OpenCL)
----------------
Anastasia wrote:
> Would we still be accepting the following:
>   global int * ptr = (global void*)0;
Yes. There is a test SemaOpenCL/null_literal.cl


================
Comment at: lib/Sema/SemaDecl.cpp:7964
+        PointeeType.getAddressSpace() == LangAS::opencl_private ||
         PointeeType.getAddressSpace() == 0)
       return InvalidAddrSpacePtrKernelParam;
----------------
Anastasia wrote:
> Could we use `LangAS::Default` instead?
done


================
Comment at: lib/Sema/SemaDecl.cpp:11846
   // an address space.
   if (T.getAddressSpace() != 0) {
     // OpenCL allows function arguments declared to be an array of a type
----------------
Anastasia wrote:
> Could we use `LangAS::Default` here too?
done


================
Comment at: lib/Sema/SemaDecl.cpp:11851
+          (T->isArrayType() ||
+           T.getAddressSpace() == LangAS::opencl_private))) {
       Diag(NameLoc, diag::err_arg_with_address_space);
----------------
Anastasia wrote:
> Would it be better to lift this into if condition of line 11846?
will do.


================
Comment at: lib/Sema/SemaType.cpp:6969
 
+  if (state.getSema().getLangOpts().OpenCL &&
+      !hasOpenCLAddressSpace && type.getAddressSpace() == 0 &&
----------------
Anastasia wrote:
> Would it be nicer to not append any address space at all neither here nor down at the end of this function and keep it default instead until the Codegen? If it's doable I would very much prefer that. It seems like it would make the implementation potentially a bit cleaner to understand and easier to improve semantical analysis. See one example of improving original type printing in my comment to the test below.
> 
> Also there are at least these 3 related bugs open currently:
> https://bugs.llvm.org//show_bug.cgi?id=33418
> https://bugs.llvm.org//show_bug.cgi?id=33419
> https://bugs.llvm.org//show_bug.cgi?id=33420
> 
> Does your change address any of those?
On the contrary, I think using default address space for automatic variable and function parameter will cause ambiguity and inconsistency in AST, making it more difficult to understand and process, and making some bug (e.g. https://bugs.llvm.org//show_bug.cgi?id=33419) unfixable. For example, `private int f(void)` and `int f(void)` will be identical in AST, therefore we cannot diagnose `private int f(void)`.

With current representation I am able to fix the 3 bugs. I will update the diff.


================
Comment at: test/SemaOpenCL/access-qualifier.cl:23
 #else
-void myReadWrite(read_write image1d_t); // expected-error {{access qualifier 'read_write' can not be used for '__read_write image1d_t' prior to OpenCL version 2.0}}
+void myReadWrite(read_write image1d_t); // expected-error {{access qualifier 'read_write' can not be used for '__private __read_write image1d_t' prior to OpenCL version 2.0}}
 #endif
----------------
Anastasia wrote:
> Ok, I think that here it would be less confusing not to add any address space since it's missing in the original source.
Although it looks strange at first sight, `__private __read_write image1d_t` is the true type of the argument. The function argument is allocated from stack and is an l-value. If we take its address, we get a private pointer.


https://reviews.llvm.org/D35082





More information about the cfe-commits mailing list