Page MenuHomePhabricator

[InstCombine] canonicalize fcmp+select to fabs
ClosedPublic

Authored by spatel on Mar 15 2018, 6:20 PM.

Details

Summary

This is based on the DAG patterns as shown in D44091. I'm hoping that we can just remove those DAG folds and always rely on IR canonicalization to handle the matching to fabs.

We would still need to delete the broken code from DAGCombiner to fix PR36600:
https://bugs.llvm.org/show_bug.cgi?id=36600

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Mar 15 2018, 6:20 PM
mike.dvoretsky added inline comments.Mar 16 2018, 6:12 AM
lib/Transforms/InstCombine/InstCombineSelect.cpp
1577 ↗(On Diff #138657)

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).

spatel added inline comments.Mar 16 2018, 7:08 AM
lib/Transforms/InstCombine/InstCombineSelect.cpp
1577 ↗(On Diff #138657)

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?

mike.dvoretsky added inline comments.Mar 16 2018, 7:53 AM
lib/Transforms/InstCombine/InstCombineSelect.cpp
1577 ↗(On Diff #138657)

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.

spatel added inline comments.Mar 16 2018, 8:38 AM
lib/Transforms/InstCombine/InstCombineSelect.cpp
1577 ↗(On Diff #138657)

Thanks. Yes, I think we should try to follow IEEE requirements here. Let me update the patch. I think this also means we have to try harder in D44521.

spatel updated this revision to Diff 138716.Mar 16 2018, 8:40 AM

Patch updated:

  1. Require nnan for all folds.
  2. Add negative test to show that we won't fold without nnan.
mike.dvoretsky accepted this revision.Mar 19 2018, 1:54 AM
This revision is now accepted and ready to land.Mar 19 2018, 1:54 AM
This revision was automatically updated to reflect the committed changes.