[PATCH] D47154: Try to make builtin address space declarations not useless

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added inline comments.


================
Comment at: lib/CodeGen/CGBuiltin.cpp:3500
+        if (auto *PtrTy = dyn_cast<llvm::PointerType>(PTy)) {
+          if (PtrTy->getAddressSpace() !=
+              ArgValue->getType()->getPointerAddressSpace()) {
----------------
Anastasia wrote:
> arsenm wrote:
> > Anastasia wrote:
> > > arsenm wrote:
> > > > Anastasia wrote:
> > > > > Would this be correct for OpenCL? Should we use  `isAddressSpaceSupersetOf` helper instead? Would it also sort the issue with constant AS (at least for OpenCL)? 
> > > > The issue I mentioned for the other builtin is that it modifies the memory, and doesn't have to do with the casting.
> > > > 
> > > > At this point the AddrSpaceCast has to be emitted. The checking if the cast is legal I guess would be in the SemaExpr part. I know at one point I was trying to use isAddressSpaceSupersetOf in rewriteBuiltinFunctionDecl, but there was some problem with that. I think it didn't make sense with the magic where the builtin without an address space is supposed to accept any address space or something along those lines.
> > > Yes, I think Sema has to check it before indeed. I am not sure it works right with OpenCL rules though for the Builtin functions.  Would it make sense to add a negative test for this then? 
> > I'm not sure what this test would look like. Do you mean a test that erroneously is accepted now?
> Ok, so at this point you are trying to change generation of `bitcast` to `addrspacecast` which makes sense to me. Do we still need a `bitcast` though?
> 
> I think `addrspacecast` can be used to convert type and address space too:
>   The ‘addrspacecast‘ instruction converts ptrval from pty in address space n to type pty2 in address space m.
> 
> It would be nice to add proper Sema checking for `Builtins` for address space of pointers in OpenCL mode, but this might be more work.
I think the canonical form is to use the bitcast for the type pointer conversion, and then separate the addrspacecast. I think instcombine splits these apart


================
Comment at: test/CodeGenOpenCL/numbered-address-space.cl:36
+#if 0
+// XXX: Should this compile?
+void test_generic_as_to_builtin_parameter_explicit_cast_numeric(__attribute__((address_space(3))) int *local_ptr, float src) {
----------------
Anastasia wrote:
> arsenm wrote:
> > Anastasia wrote:
> > > `__attribute__((address_space(N)))` is not an OpenCL feature and I think it's not specified in C either? But I think generally non matching address spaces don't compile in Clang. So it might be useful to disallow this?
> > I'm pretty sure it's a C extension. The way things seem to work now is address spaces are accepted anywhere and everywhere.
> Yes, the line below should give an error for OpenCL?
>   generic int* generic_ptr = local_ptr;
This does not error. The wording of the spec seems to leave some interpretation for other address spaces. It whitelists the valid address spaces for implicit casts, and blacklists constant for implicit or explicit casts. My reading between the lines is that an explicit cast would be OK. I think this is a separate fix since this is independent from the builtins


https://reviews.llvm.org/D47154