Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[InstCombine] canonicalize fcmp+select to fabs

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



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:

Diff Detail


Event Timeline

spatel created this revision.Mar 15 2018, 6:20 PM
mike.dvoretsky added inline comments.Mar 16 2018, 6:12 AM
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
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
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
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.