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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
