[PATCH] D44550: [InstCombine] canonicalize fcmp+select to fabs

[Mikhail Dvoretckii via Phabricator via llvm-commits]

mike.dvoretsky added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineSelect.cpp:1577
+    FCmpInst::Predicate Pred = FCI->getPredicate();
+    if (match(FCI->getOperand(1), m_AnyZeroFP())) {
+      // (X <= +/-0.0) ? (0.0 - X) : X --> fabs(X)
----------------
spatel wrote:
> mike.dvoretsky wrote:
> > I think there should be an nnan check here. The patterns in DAGCombiner check it implicitly by requiring condition codes that only occur in FP math under nnan, but this code currently doesn't require that, and all of the patterns would direct NaNs of either sign to the same option, creating the same kind of issue as the current patterns have with zero signs (and 0.0 - NaN shouldn't even do anything).
> Can you show an example of how this goes wrong with a NaN input? Is there a requirement that we preserve the sign bit of a NaN value in any of the original code sequences?
Per IEEE 754, bit operations (including fabs and copysign) consider sign bits of NaNs as if they were normal FP values. To use a modified example from PR36600:


```
#include <stdio.h>
#include <math.h>

int main(int argc, char **argv)
{
  double zero;
  sscanf(argv[1], "%lf", &zero);
  zero = 0.0 / zero; // produces a NaN if we have a zero input
  zero = copysign(zero, -1.0); // sets NaN to be negative
  zero = (zero < 0.0) ? (0.0 - zero) : zero; // NaN sign preserved
  zero = copysign(-1.0, zero); // reading NaN sign
  printf("%f\n", zero);
  return 0;
}

```
Without nsz, this produces -1.0 on zero inputs, since the NaN with the set sign bit goes through the pattern unchanged. With nsz, this patch folds it to fabs, which unsets the sign bit and leads to a 1.0 result.


https://reviews.llvm.org/D44550