[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

Sat Jul 6 19:13:34 PDT 2019

jdenny added inline comments.

================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+       !Context.getTargetInfo().hasFloat128Type() &&
+       Context.getTargetInfo().getLongDoubleWidth() != 128) ||
       (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&
----------------
ABataev wrote:
> MaskRay wrote:
> > ABataev wrote:
> > > jdenny wrote:
> > > > ABataev wrote:
> > > > > jdenny wrote:
> > > > > > ABataev wrote:
> > > > > > > jdenny wrote:
> > > > > > > > ABataev wrote:
> > > > > > > > > jdenny wrote:
> > > > > > > > > > ABataev wrote:
> > > > > > > > > > > jdenny wrote:
> > > > > > > > > > > > ABataev wrote:
> > > > > > > > > > > > > Hmm, this look strange, at least. Seems to me, in this case the size of the long double is 128 bit (copied from the host), but device reports that it does not support 128 bit double. Seems to me, it is a problem with the device configuration. Why does the host translate long double to 128 bit fp, while the device translates it to 64 bit FP?
> > > > > > > > > > > > Sorry, I think I've misunderstood what's happening here, and my fix is probably wrong.
> > > > > > > > > > > > 
> > > > > > > > > > > > For x86_64, the example from my patch summary fails as described there.  Does that work for you?
> > > > > > > > > > > > 
> > > > > > > > > > > > For powerpc64le, the reproducer I added to the test suite fails without this patch.  Shouldn't it succeed?
> > > > > > > > > > > Still, seems to me like the problem with the device config, not the original check.
> > > > > > > > > > > Still, seems to me like the problem with the device config, not the original check.
> > > > > > > > > > 
> > > > > > > > > > I'm not sure where to begin looking for that.  Can you point me in the right direction?  Thanks.
> > > > > > > > > You need to understand why host and device report different size of the type. Check how the device is configured in lib/Basic/Targets
> > > > > > > > Thanks for the pointer.  I think I understand things a bit better now.
> > > > > > > > 
> > > > > > > > Without this patch's fix, the x86_64 example from this patch's summary fails while this patch's new x86_64 test case passes.  The difference is the summary's example doesn't specify `-unknown-linux` after `x86_64`, and that's what sets `hasFloat128Type()` to true.
> > > > > > > > 
> > > > > > > > `powerpc64le-unknown-linux-gnu` does not have `__float128`, it seems.  That's why this patch's new powerpc64le test case fails without this patch's fix.
> > > > > > > > 
> > > > > > > > It seems strange to me that the code we're commenting on originally looks for the source type to be either `__float128` or 128-bit `long double`, and it then requires the target to support `__float128`.  It doesn't accept 128-bit `long double` support as sufficient.  My intention in this patch was to extend it to accept either so that all the examples above compile.  Is that too lenient?  Am I misinterpreting what's happening?
> > > > > > > > 
> > > > > > > > As for your comment about 64-bit floating point in the device translation, I haven't seen that yet.  Did I miss it?
> > > > > > > The intention of the original patch is to make host and device to have the same float128 and long double types. Device inherits those types from the host to be compatible during offloading and to correctly mangle functions.
> > > > > > > Without this we just can't generate offloading regions correctly. If the host has 128 bit long double, the device also must have 128 bit long double. 
> > > > > > > If device does not support 128bit floats, in this case device can only move the data (do load/stores ops only) and cannot do anything else.
> > > > > > Are you intentionally requiring support for `__float128` when the source type is 128-bit `long double`?  That seems to mean powerpc64le cannot offload to itself.
> > > > > No, if the host has 128 bit long double, the device must also have 128 bit long double. It has nothing to do with the float128 type itself.
> > > > What if we change the logic to the following?
> > > > 
> > > > ```
> > > > (Ty->isFloat128Type() && !Context.getTargetInfo().hasFloat128Type()) ||
> > > > (!Ty->isFloat128Type() && Ty->isRealFloatingType() &&
> > > >  Context.getTypeSize(Ty) == 128 &&
> > > >  Context.getTargetInfo().getLongDoubleWidth() != 128) 
> > > > ```
> > > > 
> > > > Maybe there's a more succinct way to check if `Ty` is `long double`....
> > > What if `Ty` is not long double, but some other FP type?
> > I know little about OpenMP... but does these lines take into account of 128-bit IBM extended double on powerpc{32,64}? It is the default representation of `long double`.
> Yes, it does, it checks for any 128bit FP type.
> What if Ty is not long double, but some other FP type?

We could use something like this to be sure it's really `long double`:

```
Ty.getUnqualifiedType() == Context.LongDoubleTy
```

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64289