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

Fri Mar 26 11:19:48 PDT 2021

Anastasia added inline comments.

================
Comment at: clang/docs/SYCLSupport.md:857
+The main address space semantic difference of SYCL mode from OpenCL is that
+SYCL doesn't assign OpenCL generic address space to a declaration's type without
+explicit address space attribute. Similar to other single-source C++-based GPU
----------------
FYI OpenCL also deduced private and global address spaces. I suggest you change this to something like:

```
SYCL doesn't perform address space qualifier inference detailed in v3.0 s6.7.8: https://www.khronos.org/registry/OpenCL/specs/3.0-unified/html/OpenCL_C.html#addr-spaces-inference`.
```

Since the inference happens in the parsing as a final step (during `CodeGen`) instead, it would be good to add that certain variables are bind to the physical global memory segments i.e. globals, static locals, etc. You already have some info at the end of the paragraph.

================
Comment at: clang/docs/SYCLSupport.md:859
+explicit address space attribute. Similar to other single-source C++-based GPU
+programming modes like OpenMP/CUDA/HIP, SYCL uses clang's "default" address
+space for types with no address space attributes. During the lowering to LLVM
----------------
I think this implied from the statement above i.e. the address spaces are not inferred.

What might be important to describe is where other objects are bound to i.e. non-globals are bound to a private memory?  You could consider refering to OpenCL spec on that since I feel it might be fairly similar?

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

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

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

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

================
Comment at: clang/docs/SYCLSupport.md:906
+
+To utilize existing clang's functionality, we re-use following OpenCL address
+space attributes in SYCL mode:
----------------
I would suggest to move this table to the top and also as an introduction to make references to OpenCL memory model and address space qualifiers to make it clear that this is reused from OpenCL.

================
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
----------------
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?

================
Comment at: clang/docs/SYCLSupport.md:922
+with any other address space (including default).
+
+### Compiler/Runtime interface
----------------
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?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99190