[PATCH] D71467: [FPEnv] Generate constrained FP comparisons from clang

Mon Dec 16 09:57:37 PST 2019

erichkeane added a comment.

In D71467#1786188 <https://reviews.llvm.org/D71467#1786188>, @uweigand wrote:

> In D71467#1785943 <https://reviews.llvm.org/D71467#1785943>, @erichkeane wrote:
>
> > I did the compare operators that didn't work right, and will do a separate patch for the fp-classification type ones: f02d6dd6c7afc08f871a623c0411f2d77ed6acf8 <https://reviews.llvm.org/rGf02d6dd6c7afc08f871a623c0411f2d77ed6acf8>
>
>
> Thanks!  Now I'm getting the correct output for the float test cases as well, and I've added them to the patch.
>
> As to fp-classification, I think there is an additional complication here:  according to IEEE and the proposed C2x standard, these builtins should **never** raise any exception, not even when receiving a signaling NaN as input.  Strictly speaking, this means that they cannot possibly be implemented in terms of any comparison operation.
>
> Now, on SystemZ (and many other platforms, I think) there are in fact specialized instructions that will implement the required semantics without raising any exceptions, but there seems to be no way to represent those at the LLVM IR level.  We'll probably need some extensions here (some new IR-level builtins?) ...
>
> (But I'd say that problem is unrelated to this patch, so I'd prefer to decouple that problem from the question of whether **this** patch is the right solution for comparisons.)

__builtin_fpclassify/isfinite/isinf/isinf_sign/isnan/isnormal/signbit are all implemented the same as the OTHER ones, except there is a strange fixup step in SEMA that removes the float->double cast.  It is IMO the wrong way to do it.

I don't think it would modify the IR at all or the AST, but I'm also working on removing that hack (which is what I meant by the fp-classification type ones).

I hope the work I've done already is sufficient to unblock this patch.

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

https://reviews.llvm.org/D71467