[PATCH] D53764: [OpenCL] Enable address spaces for references in C++

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


================
Comment at: lib/AST/Expr.cpp:1609
   case CK_AddressSpaceConversion:
-    assert(getType()->isPointerType() || getType()->isBlockPointerType());
-    assert(getSubExpr()->getType()->isPointerType() ||
-           getSubExpr()->getType()->isBlockPointerType());
-    assert(getType()->getPointeeType().getAddressSpace() !=
-           getSubExpr()->getType()->getPointeeType().getAddressSpace());
-    LLVM_FALLTHROUGH;
+    assert(/*If pointer type then addr spaces for pointees must differ*/
+           (((getType()->isPointerType() &&
----------------
Anastasia wrote:
> I don't like this assert now. Would adding extra variable be cleaner here?
Yeah, this assertion doesn't make any sense like this.  It should be checking whether the cast is a gl-value and, if so, requiring the subexpression to also be a gl-value and then asserting the difference between the type.  But you can certainly do an address-space conversion on l-values that just happen to be of pointer or block-pointer type.


================
Comment at: lib/CodeGen/CGExpr.cpp:4252
+    Address V =
+        Builder.CreateAddrSpaceCast(LV.getAddress(), ConvertType(DestTy));
+
----------------
Please use the `performAddrSpaceCast` target hook instead of directly constructing an LLVM `addrspacecast`.


================
Comment at: lib/Sema/DeclSpec.cpp:576
+      if (S.getLangOpts().OpenCLVersion < 120 &&
+          !S.getLangOpts().OpenCLCPlusPlus) {
+        DiagID = diag::err_opencl_unknown_type_specifier;
----------------
Please update the comment above this.


================
Comment at: lib/Sema/SemaDecl.cpp:7366
+             (getLangOpts().OpenCLVersion == 200 ||
+              getLangOpts().OpenCLCPlusPlus)))) {
         int Scope = NewVD->isStaticLocal() | NewVD->hasExternalStorage() << 1;
----------------
Please update the comment above this.


================
Comment at: lib/Sema/SemaInit.cpp:7614
+                        : CK_NoOp;
+      CurInit = S.ImpCastExprToType(CurInit.get(), Step->Type, CK, VK);
       break;
----------------
Please extract a function to do an l-value qualification conversion just in case we add more non-trivial conversions that we need to represent.


https://reviews.llvm.org/D53764