jdenny added inline comments.
================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:1596 << Ty << E->getSourceRange(); + if (Ty->isRealFloatingType()) { + llvm::APFloatBase::Semantics Sem = llvm::APFloatBase::SemanticsToEnum( ---------------- ABataev wrote: > jdenny wrote: > > ABataev wrote: > > > jdenny wrote: > > > > ABataev wrote: > > > > > jdenny wrote: > > > > > > ABataev wrote: > > > > > > > Why do we need all this stuff? I think the original code works > > > > > > > good here, we just need to improve the message. > > > > > > > Why do we need all this stuff? I think the original code works > > > > > > > good here, we just need to improve the message. > > > > > > > > > > > > It seems we've been miscommunicating. Let's review the discussion > > > > > > so far, point by point, and I'll show you how I arrived at this > > > > > > code. I think the first major point is as follows. > > > > > > > > > > > > I asked: > > > > > > > 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. > > > > > > > > > > > > You replied: > > > > > > > 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. > > > > > > > > > > > > I thought you were agreeing with my understanding. That is, the > > > > > > original code requires `__float128` support even when 128-bit `long > > > > > > double` is in use. That's why powerpc64le cannot offload to > > > > > > itself. How does the original code require `__float128`? It > > > > > > checks `Context.getTargetInfo().hasFloat128Type()`. As far as I > > > > > > can tell, that checks for `__float128` support, and it does not > > > > > > check for 128-bit `long double` support. That's why powerpc64le > > > > > > cannot offload to itself. > > > > > > > > > > > > I'll review other points later so we can discuss them. First, > > > > > > let's see if we can agree on this point. > > > > > > > > > > > My point about ppc64le is: > > > > > If the host code uses long double 128 bit long, tbe device long > > > > > double also must be 128 bit long. But if device does not support > > > > > 128bit FP type naturally, user cannot do any operations with it > > > > > except just load/stores. > > > > > Forget about float128 type, we talk about 128bit long double here. > > > > This patch adds new testing for powerpc64le. Without the rest of this > > > > patch (and without the `expected-error` change), should that pass? It > > > > does not for me. > > > Just like I said before, it means that your device config does not match > > > the expected one. It means that either you specified incorrect triple, or > > > the device is not configured properly. > > The triple is in the new test code. Is it incorrect? > > > > When you discussed device configuration earlier, you made a comment about > > 64 bit FP, and I couldn't find evidence of that anywhere. > > > > When I explore the device config as you suggested earlier, as far as I can > > tell, the only issue here is that it claims powerpc64le does not support > > `__float128`, but the original logic in `Sema::checkOpenMPDeviceExpr` > > requires `__float128` by checking > > `Context.getTargetInfo().hasFloat128Type()`. > Original ppc64le host reports that it supports 128bit float. The device > reports that it does not support it. It just means that the device has > different configuration than host. If yoh want the device to support 128 bit > FP as the host, you need either to specify the device correctly, orfix the > target device configuration in the lib/Basic/Target if it does not match the > expected behavior. > Check the host init for ppc64le. It sets hasFloat128 to true. For some > reason, device target config does not set this flag and reports that it does > not support 128bit FP. > Original ppc64le host reports that it supports 128bit float. > Check the host init for ppc64le. It sets hasFloat128 to true. Doesn't appear to happen for me unless I add `-target-feature +float128` as a `-cc1` option, as some existing powerpc64le clang tests do. @MaskRay, will powerpc64le set `HasFloat128=true` after your changes? 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