Anastasia added inline comments.
================ Comment at: clang/lib/Basic/Targets/SPIR.h:233 + if (Opts.HIP && Opts.CUDAIsDevice) + // Enable address space mapping from HIP to SPIR-V. + // See comment on the SPIRDefIsGenMap table. ---------------- linjamaki wrote: > Anastasia wrote: > > My guess is that this is not only HIP specific but for example the same > > applies to SYCL. > > > > I am not sure if it makes more sense to move this into a > > `BaseSPIRTargetInfo` since this is not really SPIR-V specific logic. It is > > just a clang design misalignment between two address space concepts that > > has to be addressed properly at some point. > > > The DefaultIsGeneric AS mapping is enabled for SYCL in the > BaseSPIRTargetInfo::adjust (which also means the mapping is available for > both the SPIR and SPIR-V targets). On the other hand, the AS mapping for > HIPSPV is enabled in SPIRVTargetInfo::adjust only as we intend to emit SPIR-V > only. I’m under the impression that this is what was wanted. I think the issues here is not related to the target but to the flaw in the address space design in clang. So right now all languages that don't derive the address space semantic from embedded C (SYCL/CUDA/HIP) would need to reset the address space map. See FIXME comment in the definition of `adjust`. So the right thing to do would be to set the address space map correctly straight away based on the language being compiled for which would avoid overriding this in `adjust`. But if we do override it then it makes more sense to at least unify the logic among targets. > > On the other hand, the AS mapping for HIPSPV is enabled in > SPIRVTargetInfo::adjust only as we intend to emit SPIR-V only. > I am not really sure how you would support one target only. Clang architecture (at least originally) assumes that all languages can map to all targets which in practice is not true in some cases. This is why we need to provide an address space map even for targets that have no memory segmented language compiled to it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108621/new/ https://reviews.llvm.org/D108621 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
