ABataev added inline comments.
================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594
+ !Context.getTargetInfo().hasFloat128Type() &&
+ Context.getTargetInfo().getLongDoubleWidth() != 128) ||
(Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 &&
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64289/new/
https://reviews.llvm.org/D64289
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
