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

Anastasia Stulova via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 16 03:05:32 PST 2020


Anastasia added a comment.

In D89909#2388622 <https://reviews.llvm.org/D89909#2388622>, @bader wrote:

> 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.

Did anyone conclude there that the language address spaces should be added for SYCL? I can't see any of this. In fact I don't even think there was any conclusion on the RFC. You should first make your design clear and agreed before going ahead with the implementation. I personally still think that address space AST attribute is not the right path for SYCL. And I haven't seen any evidence that it is the best solution to what you are trying to achieve.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909



More information about the llvm-commits mailing list