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

Fri Nov 27 04:30:40 PST 2020

Anastasia added a comment.

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

>> 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.
>
> I'll try to summarize my understanding of the current state. So far there were three options discussed in the mailing list thread:
>
> 1. We started with re-using OpenCL address space attributes (https://reviews.llvm.org/D80932), but there was a concern regarding deviation from OpenCL semantics in SYCL mode for these attributes. In same review, there was a proposal to implement a separate set of attributes to address this concern, which lead to the solution #2.
> 2. This patch (https://reviews.llvm.org/D89909) implements proposal from https://reviews.llvm.org/D80932 review comments to add SYCL specific attributes to avoid interference with OpenCL mode.
> 3. Bevin Hansson suggested to resolve semantic difference between SYCL and OpenCL mode for OpenCL address space attributes handling by re-using this work: https://reviews.llvm.org/D62574, which provides an infrastructure to customize address space conversion rules based on information available in ASTContext. This seems to be viable approach to implement the difference between OpenCL and SYCL modes.
>
> Open source SYCL implementation from https://github.com/intel/llvm/tree/sycl currently implements option #1 and option #2 was used in the past, so I'm quite confident that these will work. I haven't tried to prototype option #3, but as mentioned in RFC, https://reviews.llvm.org/D62574 provides necessary infrastructure to customize address space attributes already supported by the compiler.
> As we already agreed in the RFC, we should have address space attributes in AST for SYCL and the only question we need to answer is whether SYCL mode can re-using existing attributes or add something new.
>
> My current plan is to continue with option #2, unless Bevin's patch is going to be committed soon. I'd like to have something committed to unblock other changes relying on address space attributes. Let me know if you have any concerns regarding this plan.

I didn't sense any agreement or any sort of a conclusion on the RFC. Moreover, I didn't feel at all that the existing address space attribute was a good fit. I was suggesting to add a different attribute that isn't an address space attribute from Embedded C. I don't understand what you gain from the existing address space attribute at the moment. It was mentioned that the changes in the type system with address spaces is undesirable for SYCL because you indend to parse existing C++ code as-is. This contradicts the intended semantic of address spaces where the whole point of it is to modify the standard types and therefore a compilation of C++ with the standard semantic is only guaranteed when the attribute is not used at all.

I understand that you workaround this issue by adding the implicit conversions to obtain a flat address space and then recover the address spaces back in CodeGen, but this is doesn't seem like a good use of this attribute. If we all start to alter the semantic of the attribute then we won't be able to make any sense out of it. This will impact future development as any refactoring, improvements and evolution would require the understanding of the special cases that we add.  I am worried about the impact on the community for future work. This is why I was suggesting to add a completely new attribute.

Perhaps to unblock your work it would be good to have a summary of what functionality you require from the address space attribute and what needs to work differently. Then at least it would be more clear if the attribute is a good fit here and if so what difference in its semantic will be brought by SYCL customizations. Whichever route we decide to go ahead, the documentation of intended language semantic should be added somewhere publicly accessible to facilitate adequate code review and maintenance because as far as I am aware it is not documented as part of the SYCL spec or any other documentation resource.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909