[PATCH] D24011: [ConstantFold] Add a flag for ppc_fp128 constant folding, since APFloat doesn't support double-double semantic

[Hal Finkel via llvm-commits]

hfinkel added a comment.

In https://reviews.llvm.org/D24011#529901, @iteratee wrote:

> > Yes, but Hubert is right. To fully make this change, you'll need to disable constexpr evaluation of long doubles in Clang also (which I suspect might make it non-conforming).
>
>
> An incorrect frontend behavior isn't a good justification for keeping an incorrect backend behavior.


I didn't say that it was. My point is that I think you'll end up needing the full solution regardless, because the behavior will surface in places, such as constexpr evaluation in Clang, that you can't (as easily) turn off.

> 

> 

> > In the end, I don't think that going for the easy fix here is all that useful. We'll need to invest the time to making an APFloat-based double-double implementation and use that for constant folding.

> 

> 

> Correctness is its own reward.

>  As a plus, this patch doesn't interfere with someone else adding double double support to APFloat and using that in both the frontend and the backend.


It is the "someone else" I have a problem with here. I'm not okay with this patch as the desired end state of the work on this. Sure, you can disable constant folding, but that doesn't change the fact that LLVM has an APFloat with a PPCDoubleDouble mode which does not match the runtime implementation, and other uses of the LLVM libraries (i.e. Clang) will still use that implementation.

> > If the implementations in compiler-rt/lib/builtins/ppc/gcc_q*.c are good, then we could base the implementation, algorithmically, on those.

> 

> 

> Yes, that would be the way forward.


Yes, *if* they exactly match the behavior of the GCC implementations? Or do they just need to be consistent with the properties implied by numeric_limits<long double>? What are your requirements here?

> I think the important question here is if this patch is an improvement on the way to the best solution you've outlined here.

>  Tim and I both think that is.


It leaves the overall infrastructure only more-subtly broken. We might do it to make our lives easier, but it is not an improvement. As a community, sometimes we decide to say, "this has been a known problem for a long time, and so we don't want the band-aid, we'll wait for a real solution." This has certainly been a known problem for a long time, and an important question here is: Do we need to leave this as it is to motivate work on a real solution?


https://reviews.llvm.org/D24011