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

[Anastasia Stulova via Phabricator via llvm-commits]

Anastasia added a comment.

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

> In D89909#2448443 <https://reviews.llvm.org/D89909#2448443>, @Anastasia wrote:
>
>> If you prefer to continue this route then you should just document your dialect sematic somewhere publicly accessible and avoid aliasing to OpenCL, or target address spaces. So it would become a proper language dialect implementation.
>
> I've added a section to our internal design document about address space handling in SYCL mode: pull request <https://github.com/intel/llvm/pull/3027> (more readable version <https://github.com/intel/llvm/blob/249b241c4bf6514bbb2a0cbed0f4d7a0b0b3d93d/sycl/doc/CompilerAndRuntimeDesign.md#address-spaces-handling>). Please, take a look and let me know if it can be improved.

The documentation looks like a good starting point but it would be better to add it to clang docs, then we can also refine the details during the review. For now I can tell that the following points are too high-level and unclear:

> To utilize existing clang's functionality, we re-use following OpenCL address space attributes in SYCL mode with modified semantic rules and unmodified parser and IR generation components.

You should provide more details on the difference to OpenCL address spaces - in particular looking at your patch I don't think this statement is correct "unmodified <...> IR generation components."

> SYCL mode allows conversion from annotated pointer to unannotated pointer to enable integration of accelerated code with standard C++ code.

I don't understand what integration you mean. Also C++ has many different kinds of conversions, which conversions do you mean exactly?

Otherwise, the overall design of address spaces seems a lot more clear now.

Regarding `SYCLDevice` and `SYCLAddrSpaceMap` I am still not very convinced about the flow. Have you had any design discussion regarding this already that you could point to?

My main concern is that I am not sure it fits with the main design of Clang - producing IR that is to be consumed by multiple targets rather than only one specific target or even an external tool. Why do you add a triple component that is used only for one target SPIR:

1. How would existing toolchains that consume LLVM-IR for SPIR cope with the fact that the `Default` address space is different for C++ and SYCL.
2. Why is changing of `the address space map necessary for SPIR but not the other targets?

I also think it would be good if @rjmccall could look at the Codegen changes.



================
Comment at: clang/include/clang/AST/Type.h:499
+                                    other.getAddressSpace()) ||
+           (!hasAddressSpace() &&
+            (other.getAddressSpace() == LangAS::sycl_private ||
----------------
This should be done in the method above.


================
Comment at: clang/include/clang/Sema/ParsedAttr.h:624
 
+  // TBD: SYCL-specific ParsedAttr?
+  /// If this is an OpenCL address space attribute returns its SYCL
----------------
Do you still plan to resolve this?


================
Comment at: clang/lib/Basic/Targets/SPIR.h:71
     LongWidth = LongAlign = 64;
-    AddrSpaceMap = &SPIRAddrSpaceMap;
+    if (Triple.getEnvironment() == llvm::Triple::SYCLDevice) {
+      AddrSpaceMap = &SYCLAddrSpaceMap;
----------------
bader wrote:
> Anastasia wrote:
> > bader wrote:
> > > Anastasia wrote:
> > > > This deserves clarification - address space map reflect the physical address space of the target - how will upstream users get to physical address spaces of targets for SYCL?
> > > The same way. The only difference for SYCL is that "Default" language address space is mapped to "4" LLVM AS, which is equivalent to "opencl_generic" language address space.
> > If you are aiming at following the Embedded C semantic then this change is not legitimate. In Embedded C generic address space corresponds to no address space:
> > 
> > > When not specified otherwise, objects are allocated by default in a generic address space, which corresponds to the single address space of ISO/IEC 9899:1999
> > 
> > I.e. in LLVM project default address space is a flat address space of C/C++. You should either inherit it as is or use a different entry in the map... As a matter of fact, OpenCL generic address space is not the same as the generic address space of C and C++. It is only used for the pointers.
> According to my understanding Embedded C semantics is applicable to LLVM address spaces and their mapping to SPIR(-V). SPIR-V translator treats "no address space" as private, rather than generic.
I am talking about the llvm-project btw. You can't just customize it to any arbitrary GitHub project as there has to be some core design flow to be respected.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89909