MaskRay 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:
> 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`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64289/new/
https://reviews.llvm.org/D64289
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
