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