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:
> jdenny wrote:
> > 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
> > ```
> What if we have the problem with the FP type, not the long double? Shall we
> miss the check for it? I don't think so. What we need to improve, is the
> error message to describe that there is a problem with 128 bit tyoe on the
> given platform.
> What if we have the problem with the FP type, not the long double? Shall we
> miss the check for it? I don't think so.
I see there's a `llvm::fltSemantics`. Perhaps there's a way to compare that to
be sure representations are equivalent.
> What we need to improve, is the error message to describe that there is a
> problem with 128 bit tyoe on the given platform.
I agree the error message could be misleading as written now: it might claim
`long double` isn't supported when really it's just a different `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
