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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits