[PATCH] D74387: [SYCL] Defer __float128 type usage diagnostics

Wed Apr 8 14:41:11 PDT 2020

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