[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

Sun Jul 7 10:52:41 PDT 2019

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:
> > > > > > > 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?
> > > > 
> > > This is what I meant! We just missed some special target config options for the device. 
> > Thanks for the discussion.  I know what to do for the x86_64 case.  Others seem to be working on powerpc64le.  Maybe all architectures we care about (will eventually) correctly support `__float128`.  I'll abandon this patch.
> > 
> > 
> No problems. I'll fix the error message tomorrow to be more specific.
Sounds good.  Thanks again.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64289/new/

https://reviews.llvm.org/D64289