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