jdoerfert added a comment. In D74387#1969891 <https://reviews.llvm.org/D74387#1969891>, @Fznamznon wrote:
> In D74387#1967386 <https://reviews.llvm.org/D74387#1967386>, @jdoerfert wrote: > > > In D74387#1967289 <https://reviews.llvm.org/D74387#1967289>, @Fznamznon > > wrote: > > > > > In D74387#1965634 <https://reviews.llvm.org/D74387#1965634>, @jdoerfert > > > wrote: > > > > > > > In D74387#1964483 <https://reviews.llvm.org/D74387#1964483>, @Fznamznon > > > > wrote: > > > > > > > > > In D74387#1950593 <https://reviews.llvm.org/D74387#1950593>, > > > > > @jdoerfert wrote: > > > > > > > > > > > This is needed for OpenMP as well. Does it make sense to include it > > > > > > in this patch or in another one? > > > > > > > > > > > > > > > I thought OpenMP already has diagnostics for unsupported types (at > > > > > least looking into this commit > > > > > https://github.com/llvm/llvm-project/commit/123ad1969171d0b22d0c5d0ec23468586c4d8fa7). > > > > > Am I wrong? > > > > > The diagnostic which I'm implementing here is stricter than existing > > > > > OpenMP diagnostic, the main goal is do not emit unsupported type at > > > > > all. Does OpenMP need such restriction as well? > > > > > > > > > > > > OpenMP handling needs to be reverted/redone: > > > > > > > > 1. If no aux triple is available it just crashes. > > > > 2. If the unavailable type is not used in one of the pattern matched > > > > expressions it crashes (usually during instruction selection but not > > > > always). Try a call with long double arguments for example. > > > > > > > > I'm not sure this patch fits the bill but what I was thinking we need > > > > is roughly: If you have a expression with operands or function > > > > definition with return/argument types which are not supported on the > > > > target, mark the definition as unavailable with the type note you have. > > > > We should especially allow members to have unavailable types if the > > > > member is not accessed. Memcpy like operations (=mapping) are OK > > > > though. I think this should be the same for OpenMP and Sycl (and HIP, > > > > and ...). > > > > > > > > > Why we should allow members to have unavailable types if the member is > > > not accessed? I don't think that we always can do it, especially for > > > SYCL. Even if the member is not accessed directly, the whole struct with > > > unavailable type inside will get into resulting LLVM IR module anyway, > > > this can be a problem, I guess. > > > > > > On the host you know how large the type is so you can replace it in the > > device module with a placeholder of the appropriate size. You want to do > > this (in OpenMP for sure) because things you map might have constitutes you > > don't want to access on the device but you can also not (easily) split out > > of your mapped type. > > > Okay, I see. Am I right that OpenMP already has such thing implemented, but > only for functions return types? I suppose, for SYCL, we might need to > replace unsupported type in device module everywhere... > BTW, one more question, we also have a diagnostic which is emitted on > attempt to declare a variable with unsupported type inside the device code > for this __float128 type and other ones > (https://github.com/intel/llvm/pull/1465/files). Does OpenMP (and probably > HIP, CUDA etc) need such diagnostic as well? I'm not sure we want this and I'm not sure why you would. To me, it seems user hostile to disallow unsupported types categorically. We also know from our codes that people have unsupported types in structs that they would rather not refactor. Given that there is not really a need for this anyway, why should we make them? Arguably you cannot "use" unsupported types but an error like that makes sense to people. So as long as you don't use the unsupported type as an operand in an expression you should be fine. We have some detection for this in clang for OpenMP but it is not sufficient. We also should generalize this (IMHO) and stop duplicating logic between HIP/CUDA/OpenMP/SYCL/... That said, we cannot error out because the types are present but only if they are used. I would hope you would reconsider and do the same. Arguably, mapping/declaring a unsupported type explicitly could be diagnosed (with a warning) but as part of a struct I would advice against. Maybe I just don't understand. Could you elaborate why you think sycl has to forbid them categorically? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74387/new/ https://reviews.llvm.org/D74387 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
