[PATCH] D99190: WIP: [SYCL] Add design document for SYCL mode

Mon Mar 29 06:51:18 PDT 2021

bader marked 8 inline comments as done.
bader added a comment.

@Anastasia, I've addressed the comments for the address space section in https://reviews.llvm.org/D99488. Let's move discussion there.

================
Comment at: clang/docs/SYCLSupport.md:861
+space for types with no address space attributes. During the lowering to LLVM
+IR, the default address space is mapped to the SPIR generic address space.
+Declarations are assigned to the relevant memory region depending on their
----------------
Anastasia wrote:
> Ok this is an implementation details but from the language sematic it would be good to describe what logic you are expecting.
> 
> So `Default` address space is primarily used for C/C++ flat memory which means everything in standard C or C++ will be  in `Default` and this is where the local memory is allocated too.
> 
> ```
> 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 am guessing this doesn't entirely apply to SYCL? I think this would be important to clarify so it is clear what your semantic of `Default` is. It would make sense to reference OpenCL generic address space or any other documentation if you want to be concise.
I hope that the language semantics defined by the SYCL specification is clear enough. Please, see https://www.khronos.org/registry/SYCL/specs/sycl-2020/html/sycl-2020.html#_address_space_deduction for details. The only thing left for implementation to define is address space assignment for namespace scope variables. I've added a clarification for that at the end of the paragraph.

================
Comment at: clang/docs/SYCLSupport.md:863
+Declarations are assigned to the relevant memory region depending on their
+declaration context and pointers to them are cast to generic. This design has
+two important features: keeps the type system consistent with C++ on one hand
----------------
Anastasia wrote:
> Ok, I suggested to lift this to where you describe the inference. It would be good to elaborate on what objects are bound to what memory segments. You might also refer to OpenCL spec since I believe the memory segments are fairly similar. 
> 
> Can you explain this point a bit more `and pointers to them are cast to generic`? Having an example might help too.
This is a description of how CodeGen library implements Target hooks like `getGlobalVarAddressSpace`/`getASTAllocaAddressSpace`. As it's not related to SYCL, I just removed this confusing sentence.

================
Comment at: clang/docs/SYCLSupport.md:864
+declaration context and pointers to them are cast to generic. This design has
+two important features: keeps the type system consistent with C++ on one hand
+and enable tools for emitting device code aligned with SPIR memory model (and
----------------
Anastasia wrote:
> Ok, I would put the design goals to the top. 
> 
> Btw I am not sure this is the case "keeps the type system consistent with C++" since your semantic of default address spaces is different to C++. Perhaps you can elaborate more what it means...
Moved right after the links to SYCL specification.

> Btw I am not sure this is the case "keeps the type system consistent with C++" your semantic of default address spaces is different to C++

The point here is that SYCL compiler doesn't change standard C++ types by assigning non-default address space attribute implicitly. That way C++ types not using extensions are left intact.

================
Comment at: clang/docs/SYCLSupport.md:886
+
+Changing variable type has massive and destructive effect in C++. For instance
+this does not compile in C++ for OpenCL mode:
----------------
Anastasia wrote:
> I don't understand what is the message of this paragraph. The example compiles in accordance with OpenCL language semantic... Perhaps you can elaborate more.
This example demonstrates the problem with compiling C++ code when address space type qualifiers are inferred.

> The example compiles in accordance with OpenCL language semantic...

https://godbolt.org/z/9jzxK5xc4 - ToT clang doesn't compile this example.

================
Comment at: clang/docs/SYCLSupport.md:919
+> **NOTE**: although SYCL device compiler supports
+`__attribute__((opencl_constant))`, the use of this attribute is limited within
+SYCL implementation. An OpenCL constant pointer can not be casted to a pointer
----------------
Anastasia wrote:
> I am not sure what limited means? I would suggest being more specific and state something like that there is no binding to the constant memory region. Unless this is something that you intend to support later?
I removed this confusing section. It describes the functionality, which is not being considered for upstream yet.

================
Comment at: clang/docs/SYCLSupport.md:922
+with any other address space (including default).
+
+### Compiler/Runtime interface
----------------
Anastasia wrote:
> It would also be good to see some clarifications regarding your address space overlapping rules and the implicit/explicit conversions and whether there is any behavior that doesn't comply to embedded C/OpenCL C: https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#addr-spaces-conversions
> 
> Also do you have any constraints on variable initialization wrt address spaces?
Added.

I don't have anything in addition to SYCL and SPIR constraints.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99190