Anastasia 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)
----------------
Would we still be accepting the following:
  global int * ptr = (global void*)0;


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


================
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
----------------
Could we use `LangAS::Default` here too?


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


================
Comment at: lib/Sema/SemaType.cpp:6969
 
+  if (state.getSema().getLangOpts().OpenCL &&
+      !hasOpenCLAddressSpace && type.getAddressSpace() == 0 &&
----------------
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?


================
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
----------------
Ok, I think that here it would be less confusing not to add any address space 
since it's missing in the original source.


https://reviews.llvm.org/D35082



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to