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

Thu Apr 9 06:28:41 PDT 2020

Fznamznon added a comment.

In D74387#1970374 <https://reviews.llvm.org/D74387#1970374>, @jdoerfert wrote:

> 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?

Roughly speaking, SYCL is a wrapper over OpenCL. SYCL device compiler should be able to produce device code module in a form acceptable by OpenCL backends. For this purpose we use SPIR-V intermediate language (https://www.khronos.org/registry/spir-v/specs/unified1/SPIRV.html). We transform LLVM IR emitted by clang (in SYCL device mode) into SPIR-V, then feed it to OpenCL backends. To be able to do it, produced SPIR-V must be valid and do not require additional features/capabilities comparing with SPIR-V produced from pure OpenCL, otherwise OpenCL backends just don't work with it. Nor OpenCL neither SPIRV doesn't support `__float128` type, for example. From SPIR-V spec:

> Scalar floating-point types can be parameterized only as 32 bit, plus any additional sizes enabled by capabilities (i.e. 16 and 64 for some devices).

Right now It is not possible to produce valid SPIR-V from LLVM IR containing unsupported types. We use official Khronos SPIRV translator (https://github.com/KhronosGroup/SPIRV-LLVM-Translator). SPIR-V translator relies on clang to prohibit unsupported features, so they are not expected in LLVM IR. That is why we might need completely prohibit (or maybe we need to replace it in resulting LLVM module completely if it is possible?) now.

I'm also curious why OpenMP can just allow presence of unsupported type in the resulting module? Doesn't it produce any problems while compiling code by device-specific back-end for some specific device which don't support such type?

I also think that we need to generalize approaches between OpenMP/SYCL/CUDA/HIP. We can start with generalized diagnostic which points to using of unsupported type at least, then we can add additional restriction for SYCL (or other programming models) if we need one. @bader , @erichkeane please comment if you don't agree.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74387/new/

https://reviews.llvm.org/D74387