[InstCombine] canonicalize fcmp+select to fabs

[spatel]

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

[mike.dvoretsky]

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]

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]

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]

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]

Patch updated:

1. Require nnan for all folds.
2. Add negative test to show that we won't fold without nnan.

llvm/trunk/lib/Transforms/InstCombine/InstCombineSelect.cpp

if (FCI->getOperand(0) == TrueVal && FCI->getOperand(1) == FalseVal) {
                                             FCI->getName() + ".inv");
 
         return SelectInst::Create(NewCond, FalseVal, TrueVal,
                                   SI.getName() + ".p");
       }
 
       // NOTE: if we wanted to, this is where to detect MIN/MAX
     }
-    // NOTE: if we wanted to, this is where to detect ABS
+
+    // Canonicalize select with fcmp to fabs(). -0.0 makes this tricky. We need
+    // fast-math-flags (nsz) or fsub with +0.0 (not fneg) for this to work. We
+    // also require nnan because we do not want to unintentionally change the
+    // sign of a NaN value.
+    Value *X = FCI->getOperand(0);
+    FCmpInst::Predicate Pred = FCI->getPredicate();
+    if (match(FCI->getOperand(1), m_AnyZeroFP()) && FCI->hasNoNaNs()) {
+      // (X <= +/-0.0) ? (0.0 - X) : X --> fabs(X)
+      // (X >  +/-0.0) ? X : (0.0 - X) --> fabs(X)
+      if ((X == FalseVal && match(TrueVal, m_FSub(m_Zero(), m_Specific(X))) &&
+          Pred == FCmpInst::FCMP_OLE) ||
+          (X == TrueVal && match(FalseVal, m_FSub(m_Zero(), m_Specific(X))) &&
+          Pred == FCmpInst::FCMP_OGT)) {
+        Value *Fabs = Builder.CreateIntrinsic(Intrinsic::fabs, { X }, FCI);
+        return replaceInstUsesWith(SI, Fabs);
+      }
+      // With nsz:
+      // (X <  +/-0.0) ? -X : X --> fabs(X)
+      // (X <= +/-0.0) ? -X : X --> fabs(X)
+      // (X >  +/-0.0) ? X : -X --> fabs(X)
+      // (X >= +/-0.0) ? X : -X --> fabs(X)
+      if (FCI->hasNoSignedZeros() &&
+          ((X == FalseVal && match(TrueVal, m_FNeg(m_Specific(X))) &&
+            (Pred == FCmpInst::FCMP_OLT || Pred == FCmpInst::FCMP_OLE)) ||
+           (X == TrueVal && match(FalseVal, m_FNeg(m_Specific(X))) &&
+            (Pred == FCmpInst::FCMP_OGT || Pred == FCmpInst::FCMP_OGE)))) {
+        Value *Fabs = Builder.CreateIntrinsic(Intrinsic::fabs, { X }, FCI);
+        return replaceInstUsesWith(SI, Fabs);
+      }
+    }
   }
 
   // See if we are selecting two values based on a comparison of the two values.
   if (ICmpInst *ICI = dyn_cast<ICmpInst>(CondVal))
     if (Instruction *Result = foldSelectInstWithICmp(SI, ICI))
       return Result;
 
   if (Instruction *Add = foldAddSubSelect(SI, Builder))

llvm/trunk/test/Transforms/InstCombine/fabs.ll

 ; CHECK-NEXT:    ret double [[FABS]]
 ;
   %fpext = fpext float %x to double
   %fabs = call double @llvm.fabs.f64(double %fpext)
   store volatile double %fpext, double* undef
   ret double %fabs
 }
 
-; X <= 0.0 ? (0.0 - X) : X --> fabs(X)
+; Negative test for the fabs folds below: we require nnan, so
+; we won't always clear the sign bit of a NaN value.
 
 define double @select_fcmp_ole_zero(double %x) {
 ; CHECK-LABEL: @select_fcmp_ole_zero(
-; CHECK-NEXT:    [[LEZERO:%.*]] = fcmp nnan ole double [[X:%.*]], 0.000000e+00
+; CHECK-NEXT:    [[LEZERO:%.*]] = fcmp ole double [[X:%.*]], 0.000000e+00
 ; CHECK-NEXT:    [[NEGX:%.*]] = fsub double 0.000000e+00, [[X]]
 ; CHECK-NEXT:    [[FABS:%.*]] = select i1 [[LEZERO]], double [[NEGX]], double [[X]]
 ; CHECK-NEXT:    ret double [[FABS]]
 ;
+  %lezero = fcmp ole double %x, 0.0
+  %negx = fsub double 0.0, %x
+  %fabs = select i1 %lezero, double %negx, double %x
+  ret double %fabs
+}
+
+; X <= 0.0 ? (0.0 - X) : X --> fabs(X)
+
+define double @select_fcmp_nnan_ole_zero(double %x) {
+; CHECK-LABEL: @select_fcmp_nnan_ole_zero(
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan double @llvm.fabs.f64(double [[X:%.*]])
+; CHECK-NEXT:    ret double [[TMP1]]
+;
   %lezero = fcmp nnan ole double %x, 0.0
   %negx = fsub double 0.0, %x
   %fabs = select i1 %lezero, double %negx, double %x
   ret double %fabs
 }
 
 ; X <= -0.0 ? (0.0 - X) : X --> fabs(X)
 
-define float @select_fcmp_ole_negzero(float %x) {
-; CHECK-LABEL: @select_fcmp_ole_negzero(
-; CHECK-NEXT:    [[LEZERO:%.*]] = fcmp nnan ole float [[X:%.*]], -0.000000e+00
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub float 0.000000e+00, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select i1 [[LEZERO]], float [[NEGX]], float [[X]]
-; CHECK-NEXT:    ret float [[FABS]]
+define float @select_fcmp_nnan_ole_negzero(float %x) {
+; CHECK-LABEL: @select_fcmp_nnan_ole_negzero(
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan float @llvm.fabs.f32(float [[X:%.*]])
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %lezero = fcmp nnan ole float %x, -0.0
   %negx = fsub float 0.0, %x
   %fabs = select i1 %lezero, float %negx, float %x
   ret float %fabs
 }
 
 ; X > 0.0 ? X : (0.0 - X) --> fabs(X)
 
-define fp128 @select_fcmp_ogt_zero(fp128 %x) {
-; CHECK-LABEL: @select_fcmp_ogt_zero(
-; CHECK-NEXT:    [[GTZERO:%.*]] = fcmp nnan ogt fp128 [[X:%.*]], 0xL00000000000000000000000000000000
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub fp128 0xL00000000000000000000000000000000, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select i1 [[GTZERO]], fp128 [[X]], fp128 [[NEGX]]
-; CHECK-NEXT:    ret fp128 [[FABS]]
+define fp128 @select_fcmp_nnan_ogt_zero(fp128 %x) {
+; CHECK-LABEL: @select_fcmp_nnan_ogt_zero(
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan fp128 @llvm.fabs.f128(fp128 [[X:%.*]])
+; CHECK-NEXT:    ret fp128 [[TMP1]]
 ;
   %gtzero = fcmp nnan ogt fp128 %x, zeroinitializer
   %negx = fsub fp128 zeroinitializer, %x
   %fabs = select i1 %gtzero, fp128 %x, fp128 %negx
   ret fp128 %fabs
 }
 
 ; X > -0.0 ? X : (0.0 - X) --> fabs(X)
 
-define half @select_fcmp_ogt_negzero(half %x) {
-; CHECK-LABEL: @select_fcmp_ogt_negzero(
-; CHECK-NEXT:    [[GTZERO:%.*]] = fcmp nnan ogt half [[X:%.*]], 0xH8000
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub half 0xH0000, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select i1 [[GTZERO]], half [[X]], half [[NEGX]]
-; CHECK-NEXT:    ret half [[FABS]]
+define half @select_fcmp_nnan_ogt_negzero(half %x) {
+; CHECK-LABEL: @select_fcmp_nnan_ogt_negzero(
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan half @llvm.fabs.f16(half [[X:%.*]])
+; CHECK-NEXT:    ret half [[TMP1]]
 ;
   %gtzero = fcmp nnan ogt half %x, -0.0
   %negx = fsub half 0.0, %x
   %fabs = select i1 %gtzero, half %x, half %negx
   ret half %fabs
 }
 
 ; X < 0.0 ? -X : X --> fabs(X)
 
-define double @select_fcmp_nsz_olt_zero(double %x) {
-; CHECK-LABEL: @select_fcmp_nsz_olt_zero(
-; CHECK-NEXT:    [[LTZERO:%.*]] = fcmp nnan nsz olt double [[X:%.*]], 0.000000e+00
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub double -0.000000e+00, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select i1 [[LTZERO]], double [[NEGX]], double [[X]]
-; CHECK-NEXT:    ret double [[FABS]]
+define double @select_fcmp_nnan_nsz_olt_zero(double %x) {
+; CHECK-LABEL: @select_fcmp_nnan_nsz_olt_zero(
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz double @llvm.fabs.f64(double [[X:%.*]])
+; CHECK-NEXT:    ret double [[TMP1]]
 ;
   %ltzero = fcmp nnan nsz olt double %x, 0.0
   %negx = fsub double -0.0, %x
   %fabs = select i1 %ltzero, double %negx, double %x
   ret double %fabs
 }
 
 ; X < -0.0 ? -X : X --> fabs(X)
 
-define float @select_fcmp_nsz_olt_negzero(float %x) {
-; CHECK-LABEL: @select_fcmp_nsz_olt_negzero(
-; CHECK-NEXT:    [[LTZERO:%.*]] = fcmp nnan ninf nsz olt float [[X:%.*]], -0.000000e+00
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub float -0.000000e+00, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select i1 [[LTZERO]], float [[NEGX]], float [[X]]
-; CHECK-NEXT:    ret float [[FABS]]
+define float @select_fcmp_nnan_nsz_olt_negzero(float %x) {
+; CHECK-LABEL: @select_fcmp_nnan_nsz_olt_negzero(
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan ninf nsz float @llvm.fabs.f32(float [[X:%.*]])
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %ltzero = fcmp nnan nsz ninf olt float %x, -0.0
   %negx = fsub float -0.0, %x
   %fabs = select i1 %ltzero, float %negx, float %x
   ret float %fabs
 }
 
 ; X <= 0.0 ? -X : X --> fabs(X)
 
-define double @select_fcmp_nsz_ole_zero(double %x) {
-; CHECK-LABEL: @select_fcmp_nsz_ole_zero(
-; CHECK-NEXT:    [[LEZERO:%.*]] = fcmp fast ole double [[X:%.*]], 0.000000e+00
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub double -0.000000e+00, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select i1 [[LEZERO]], double [[NEGX]], double [[X]]
-; CHECK-NEXT:    ret double [[FABS]]
+define double @select_fcmp_nnan_nsz_ole_zero(double %x) {
+; CHECK-LABEL: @select_fcmp_nnan_nsz_ole_zero(
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast double @llvm.fabs.f64(double [[X:%.*]])
+; CHECK-NEXT:    ret double [[TMP1]]
 ;
   %lezero = fcmp fast ole double %x, 0.0
   %negx = fsub double -0.0, %x
   %fabs = select i1 %lezero, double %negx, double %x
   ret double %fabs
 }
 
 ; X <= -0.0 ? -X : X --> fabs(X)
 
-define float @select_fcmp_nsz_ole_negzero(float %x) {
-; CHECK-LABEL: @select_fcmp_nsz_ole_negzero(
-; CHECK-NEXT:    [[LEZERO:%.*]] = fcmp nnan nsz ole float [[X:%.*]], -0.000000e+00
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub float -0.000000e+00, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select i1 [[LEZERO]], float [[NEGX]], float [[X]]
-; CHECK-NEXT:    ret float [[FABS]]
+define float @select_fcmp_nnan_nsz_ole_negzero(float %x) {
+; CHECK-LABEL: @select_fcmp_nnan_nsz_ole_negzero(
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz float @llvm.fabs.f32(float [[X:%.*]])
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %lezero = fcmp nnan nsz ole float %x, -0.0
   %negx = fsub float -0.0, %x
   %fabs = select i1 %lezero, float %negx, float %x
   ret float %fabs
 }
 
 ; X > 0.0 ? X : (0.0 - X) --> fabs(X)
 
-define <2 x float> @select_fcmp_nsz_ogt_zero(<2 x float> %x) {
-; CHECK-LABEL: @select_fcmp_nsz_ogt_zero(
-; CHECK-NEXT:    [[GTZERO:%.*]] = fcmp nnan nsz arcp ogt <2 x float> [[X:%.*]], zeroinitializer
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub <2 x float> <float -0.000000e+00, float -0.000000e+00>, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select <2 x i1> [[GTZERO]], <2 x float> [[X]], <2 x float> [[NEGX]]
-; CHECK-NEXT:    ret <2 x float> [[FABS]]
+define <2 x float> @select_fcmp_nnan_nsz_ogt_zero(<2 x float> %x) {
+; CHECK-LABEL: @select_fcmp_nnan_nsz_ogt_zero(
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz arcp <2 x float> @llvm.fabs.v2f32(<2 x float> [[X:%.*]])
+; CHECK-NEXT:    ret <2 x float> [[TMP1]]
 ;
   %gtzero = fcmp nnan nsz arcp ogt <2 x float> %x, zeroinitializer
   %negx = fsub <2 x float> <float -0.0, float -0.0>, %x
   %fabs = select <2 x i1> %gtzero, <2 x float> %x, <2 x float> %negx
   ret <2 x float> %fabs
 }
 
 ; X > -0.0 ? X : (0.0 - X) --> fabs(X)
 
-define half @select_fcmp_nsz_ogt_negzero(half %x) {
-; CHECK-LABEL: @select_fcmp_nsz_ogt_negzero(
-; CHECK-NEXT:    [[GTZERO:%.*]] = fcmp fast ogt half [[X:%.*]], 0xH8000
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub half 0xH0000, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select i1 [[GTZERO]], half [[X]], half [[NEGX]]
-; CHECK-NEXT:    ret half [[FABS]]
+define half @select_fcmp_nnan_nsz_ogt_negzero(half %x) {
+; CHECK-LABEL: @select_fcmp_nnan_nsz_ogt_negzero(
+; CHECK-NEXT:    [[TMP1:%.*]] = call fast half @llvm.fabs.f16(half [[X:%.*]])
+; CHECK-NEXT:    ret half [[TMP1]]
 ;
   %gtzero = fcmp fast ogt half %x, -0.0
   %negx = fsub half 0.0, %x
   %fabs = select i1 %gtzero, half %x, half %negx
   ret half %fabs
 }
 
 ; X > 0.0 ? X : (0.0 - X) --> fabs(X)
 
-define <2 x double> @select_fcmp_nsz_oge_zero(<2 x double> %x) {
-; CHECK-LABEL: @select_fcmp_nsz_oge_zero(
-; CHECK-NEXT:    [[GEZERO:%.*]] = fcmp reassoc nnan nsz oge <2 x double> [[X:%.*]], zeroinitializer
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub <2 x double> <double -0.000000e+00, double -0.000000e+00>, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select <2 x i1> [[GEZERO]], <2 x double> [[X]], <2 x double> [[NEGX]]
-; CHECK-NEXT:    ret <2 x double> [[FABS]]
+define <2 x double> @select_fcmp_nnan_nsz_oge_zero(<2 x double> %x) {
+; CHECK-LABEL: @select_fcmp_nnan_nsz_oge_zero(
+; CHECK-NEXT:    [[TMP1:%.*]] = call reassoc nnan nsz <2 x double> @llvm.fabs.v2f64(<2 x double> [[X:%.*]])
+; CHECK-NEXT:    ret <2 x double> [[TMP1]]
 ;
   %gezero = fcmp nnan nsz reassoc oge <2 x double> %x, zeroinitializer
   %negx = fsub <2 x double> <double -0.0, double -0.0>, %x
   %fabs = select <2 x i1> %gezero, <2 x double> %x, <2 x double> %negx
   ret <2 x double> %fabs
 }
 
 ; X > -0.0 ? X : (0.0 - X) --> fabs(X)
 
-define half @select_fcmp_nsz_oge_negzero(half %x) {
-; CHECK-LABEL: @select_fcmp_nsz_oge_negzero(
-; CHECK-NEXT:    [[GEZERO:%.*]] = fcmp nnan nsz oge half [[X:%.*]], 0xH8000
-; CHECK-NEXT:    [[NEGX:%.*]] = fsub half 0xH8000, [[X]]
-; CHECK-NEXT:    [[FABS:%.*]] = select i1 [[GEZERO]], half [[X]], half [[NEGX]]
-; CHECK-NEXT:    ret half [[FABS]]
+define half @select_fcmp_nnan_nsz_oge_negzero(half %x) {
+; CHECK-LABEL: @select_fcmp_nnan_nsz_oge_negzero(
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan nsz half @llvm.fabs.f16(half [[X:%.*]])
+; CHECK-NEXT:    ret half [[TMP1]]
 ;
   %gezero = fcmp nnan nsz oge half %x, -0.0
   %negx = fsub half -0.0, %x
   %fabs = select i1 %gezero, half %x, half %negx
   ret half %fabs
 }