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

Mon Nov 30 12:16:26 PST 2020

bader added a comment.

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

We re-use a lot functionality across clang libraries: parser (re-use existing attributes), semantic analysis (e.g. applying multiple AS attributes, type conversion, etc), code generation (mapping address space attribute to LLVM address spaces required by LLVM-SPIRV translator). We will have to duplicate this functionality for a new attribute set.

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

Right, but I don't think it's related to the address space attributes. It was mentioned in the context of re-using OpenCL *mode* for SYCL device compilation, which modifies types which does use address space attribute explicitly. "Existing C++ code" doesn't use address space attributes and our solution does differentiate explicitly annotated type. The difference with OpenCL mode is that SYCL doesn't change types by modifying "default" address space attribute and allows conversion from/to "default" address space. As I mentioned in RFC, according to my understanding this patch doesn't contradict Embedded C requirements regarding address space attribute usage. I think the spec allows an implementation to define conversion rules between address spaces and doesn't imply type change based on the declaration scope - it's OpenCL specific behavior.

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

I'm not sure we understand it the same way. We use this attribute the same way OpenCL does. Compiler adds implicit conversion for mismatched types in OpenCL mode as well, the difference is in additional type modifications clang applies in OpenCL, which are not applied in SYCL mode to keep C++ templates functional.

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

You suggest adding new parsed attributes, right? This patch adds new semantic attributes, so they should not alter semantic of any existing attributes anymore.

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

The only difference directly related to clang's "opencl_*" address space attributes is that SYCL allows conversion between types with OpenCL address space attributes and "default" address space, but it's an implementation detail. The main sematic difference is that SYCL doesn't change "default" address space and it's "visible" with template metaprogramming or type deduction at compile time. It looks like the best place to describe this difference is a separate document similar to https://clang.llvm.org/docs/OpenCLSupport.html, right?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909