[PATCH] D89909: [SYCL] Implement SYCL address space attributes handling

Wed Nov 11 06:44:27 PST 2020

bader added a subscriber: sdmitriev.
bader added a comment.

In D89909#2353931 <https://reviews.llvm.org/D89909#2353931>, @Anastasia wrote:

> In the RFC it has been discussed to either use target address spaces or perhaps to introduce a new attribute to reflect a semantic needed for SYCL, but it seems to me in this change you are building on top of the existing language address space attribute with new entries for SYCL.
>
> http://lists.llvm.org/pipermail/cfe-dev/2020-August/066632.html

Right, this patch adds new semantic attributes for SYCL and I left a comment to decide whether we need a separate representation for parsed attribute and spelling as well. Do you suggest adding SYCL specific spelling for these attributes?

WRT to a solution built on top of https://reviews.llvm.org/D62574, I'm hesitate investing into this direction as it's not clear to me when we can get this patch in (it's on review for 17 months already). I'd like to make some progress with address space attributes to unblock upstreaming of other changes depending on this change.

> Has there been any other discussion regarding the approach which is not captured in the cfe-dev thread I have linked?

I think everything is captured in the mailing list thread.

================
Comment at: clang/lib/CodeGen/CGCall.cpp:4596
+                                            IRFuncTy->getParamType(FirstIRArg));
+        }
+
----------------
rjmccall wrote:
> This seems problematic; code like this shouldn't be necessary in every place that uses a pointer value.  The general rule needs to be that expression emission produces a value of its expected type, so the places that emit pointers need to be casting to the generic address space.  We can avoid that in some special expression emitters, like maybe EmitLValue, as long as the LValue records what happened and can apply the appropriate transform later.
> 
> Also, in IRGen we work as much as possible with AST address spaces rather than IR address spaces.  That is, code like this that's checking for address space mismatches needs to be comparing the source AST address space with the destination AST address space and calling a method on the TargetInfo when a mismatch is detected, rather than comparing IR address spaces and directly building an `addrspacecast`.  The idea is that in principle a target should be able to have two logical address spaces that are actually implemented in IR with the same underlying address space, with some arbitrary transform between them.
@erichkeane, @asavonic, could you help with addressing this concern, please?

================
Comment at: clang/lib/CodeGen/CGExpr.cpp:4554
+
+      // Language rules define if it is legal to cast from one address space
+      // to another, and which address space we should use as a "common
----------------
Anastasia wrote:
> What language rules?
I suppose it's a generic note and author didn't meant some particular language here. This change was contributed by @sdmitriev. 
@sdmitriev, could you clarify, please?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909