bader added a comment. In D89909#2439837 <https://reviews.llvm.org/D89909#2439837>, @Anastasia wrote:
> In D89909#2423750 <https://reviews.llvm.org/D89909#2423750>, @bader wrote: > >>> It was mentioned that the changes in the type system with address spaces is >>> undesirable for SYCL because you indend to parse existing C++ code as-is. >>> This contradicts the intended semantic of address spaces where the whole >>> point of it is to modify the standard types and therefore a compilation of >>> C++ with the standard semantic is only guaranteed when the attribute is not >>> used at all. >> >> Right, but I don't think it's related to the address space attributes. It >> was mentioned in the context of re-using OpenCL *mode* for SYCL device >> compilation, which modifies types which does use address space attribute >> explicitly. "Existing C++ code" doesn't use address space attributes and our >> solution does differentiate explicitly annotated type. The difference with >> OpenCL mode is that SYCL doesn't change types by modifying "default" address >> space attribute and allows conversion from/to "default" address space. As I >> mentioned in RFC, according to my understanding this patch doesn't >> contradict Embedded C requirements regarding address space attribute usage. >> I think the spec allows an implementation to define conversion rules between >> address spaces and doesn't imply type change based on the declaration scope >> - it's OpenCL specific behavior. > > Ok, if you plan to follow the Embedded C semantic (which your current patch > confirms) then you should just reuse the existing target address spaces and > extend the implementation with the ability to specify the relation among > address spaces. The patch that you have mentioned earlier > https://reviews.llvm.org/D62574 is providing this logic already and it looks > good aside from testing. I would suggest to discuss with @ebevhan the > timeline for committing it as the testing could be done using SYCL > implementation. Alternatively, you could consider reusing the relevant parts > of the patch if @ebevhan has no objections to that. Let me check that I understand your proposal correctly. Do suggest that we use `__attribute__((address_space(N)))` attribute and define the relation between the set of these? Unfortunately I won't work because SYCL supports multiple targets and we need to use address space maps to correctly map attributes in LLVM IR. Targets might use different different llvm address spaces numbers for memory accessible within a work-item. If I understand it correctly, `__attribute__((address_space(N)))` will be mapped to `addrspace(N)` in LLVM and this mapping can't be customized for different targets. Right? I actually proposed a bit different approach in RFC. https://reviews.llvm.org/D62574 "moves QualType/Qualifiers accessors which deal with qualifier relations (such as compatiblyIncludes, etc.) to ASTContext, as Qualifiers cannot be made aware of the relations between address spaces on their own". It allows customization based on the language mode as well, so my suggestion was to set "default" address space as a superset for opencl_global, opencl_local and opencl_private in SYCL mode. Using this approach we re-use existing attributes and don't impact other language modes. Does it make sense to you? ================ Comment at: clang/lib/Basic/Targets.cpp:577 - case llvm::Triple::spir: { - if (Triple.getOS() != llvm::Triple::UnknownOS || - Triple.getEnvironment() != llvm::Triple::UnknownEnvironment) ---------------- Anastasia wrote: > We should not allow any arbitrary OS/Environment for SPIR. @Anastasia, could you elaborate on that, please? Why can't we allow arbitrary values for these triple components? The only reason I'm aware of is that LLVM-SPIRV-Translator was checking OS triple component and rejected LLVM module if value was not "unknow". Today it checks only "architecture' component and accepts only "spir" architecture. I personally think that even this restriction is artificial and LLVM module shouldn't be rejected based on the triple values, but it's a separate discussion. I use additional environment component to choose between address space maps. We could update default address space in SPIRAddrSpaceMap directly, but IIRC, it caused some issues for OpenCL C++ mode, which uses Default address space instead of `opencl_private` in some cases. Do you know if there is a better way to communicate the difference in Default address space to SPIRTargetInfo object? ================ Comment at: clang/lib/Basic/Targets/SPIR.h:71 LongWidth = LongAlign = 64; - AddrSpaceMap = &SPIRAddrSpaceMap; + if (Triple.getEnvironment() == llvm::Triple::SYCLDevice) { + AddrSpaceMap = &SYCLAddrSpaceMap; ---------------- 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. ================ Comment at: clang/lib/CodeGen/TargetInfo.cpp:10019 + +LangAS SPIRTargetCodeGenInfo::getGlobalVarAddressSpace(CodeGenModule &CGM, + const VarDecl *D) const { ---------------- Anastasia wrote: > You should document this functionality. I don't think the dialects will need > to call this function. This method is documented in the header file: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/TargetInfo.h#L244. I'll add SPIR specific requirements implemented in this overload. ================ Comment at: clang/lib/CodeGen/TargetInfo.cpp:10035 + if (CGM.isTypeConstant(D->getType(), false)) { + if (auto ConstAS = CGM.getTarget().getConstantAddressSpace()) + return ConstAS.getValue(); ---------------- Anastasia wrote: > You should not have constant address space in SYCL! According to my understanding it's not about "constant" address space like in OpenCL. This method returns LLVM address space for constants (like literals: `56`, "hello"). I think it might be better to split this patch into two and move enabling regular C++ for SPIR to a separate patch. I'll try to make this change if there are objections. 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
