[PATCH] D60193: [OpenCL] Added addrspace_cast operator

[Marco Antognini via Phabricator via cfe-commits]

mantognini added a comment.

Thanks for your clarifications and updates. Just one tiny question about a test file, but otherwise LGTM.



================
Comment at: clang/test/SemaOpenCLCXX/addrspace_cast.cl:19-24
+template <class T>
+void test_temp(__global int *par) {
+  T *var1 = addrspace_cast<T *>(par);
+  __private T *var2 = addrspace_cast<__private T *>(par);
+  T var3 = addrspace_cast<T>(par);
+}
----------------
Does this test anything since it's not instantiated?


================
Comment at: lib/Sema/SemaCast.cpp:285
+    return Op.complete(CXXAddrspaceCastExpr::Create(Context, Op.ResultType,
+                                    Op.ValueKind, Op.SrcExpr.get(),
+                                                  DestTInfo,
----------------
Anastasia wrote:
> mantognini wrote:
> > The formatting looks a bit funny here.
> I agree, I just matched the style of the old code to keep it coherent. Although perhaps I should rather adhere to the current style?
I don't have a strong opinion on what's best.


================
Comment at: lib/Sema/SemaCast.cpp:2338
   auto DestPointeeType = DestPtrType->getPointeeType();
   if (SrcPointeeType.getAddressSpace() == DestPointeeType.getAddressSpace())
     return TC_NotApplicable;
----------------
Anastasia wrote:
> mantognini wrote:
> > Wouldn't this limit usage of the cast unnecessarily? I'm thinking this could be transformed to a NOP, which could be beneficial to make (template) code simpler to write.
> I am not sure what you mean. I have added the test for templates and it caught a bug in lib/AST/Expr.cpp with assert condition.
> 
> However, now that I think about this more, I believe we should allow compiling this?
> 
> ```
> __private int* i;
> addrspace_cast<private int*>(i);
> ```
> 
> Currently it outputs an error.
Yes, that's what I meant. (Although I see it's not part of this review so I'm not saying this should be changed now.)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60193/new/

https://reviews.llvm.org/D60193