[InstCombine] x /c fabs(x) -> copysign(1.0, x)

[xbolva00]

Summary

x / fabs(x) -> copysign(1.0, x)
fabs(x) / x -> copysign(1.0, x)

[RKSimon]

vector tests?

[xbolva00]

Added vector test

[spatel]

lib/Transforms/InstCombine/InstCombineMulDivRem.cpp

1237

Do we want to handle the commuted pattern too?
fabs(X) / X

1240

Do we need the one-use restriction?

test/Transforms/InstCombine/fabs-copysign.ll

42

This is supposed to be an extra use test, but there's no extra use?

[xbolva00]

Addressed review notes

[xbolva00]

lib/Transforms/InstCombine/InstCombineMulDivRem.cpp

1240

Dropped.

[xbolva00]

lib/Transforms/InstCombine/InstCombineMulDivRem.cpp

1246

I tried

match(&I, m_FDiv(m_Value(X), m_Intrinsic<Intrinsic::fabs>(m_Specific(X)))) and it failed to match.. uh.

(this blocked me to possibly introduce m_c_FDiv)

[spatel]

lib/Transforms/InstCombine/InstCombineMulDivRem.cpp

1246

You need to use m_Deferred() for that case when matching commutatively (cc @lebedev.ri ).
But I don't think it's worth adding a commutative matcher for fdiv since it isn't a commutative operation. Not too many cases like this where it can be used?

test/Transforms/InstCombine/fabs-copysign.ll

21–29

Please push the new tests with baseline CHECKs as a preliminary step.

[xbolva00]

Use m_Deferred

[xbolva00]

lib/Transforms/InstCombine/InstCombineMulDivRem.cpp

1246

Thanks!

(and agree)

[spatel]

LGTM

[xbolva00]

Thanks

[spatel]

In D65898#1625141, @xbolva00 wrote:

Thanks

Wait - there was a vector test here at 1 point, but now it's gone? Please adjust the type on one of the positive tests, so we still have that coverage.

[xbolva00]

Uh, that’s interesting how that test was lost.

I will add it again, sorry.

[xbolva00]

Readded vector tests

lib/Transforms/InstCombine/InstCombineMulDivRem.cpp

Instruction *InstCombiner::visitFDiv(BinaryOperator &I) {
   // We can ignore the possibility that X is infinity because INF/INF is NaN.
   if (I.hasNoNaNs() && I.hasAllowReassoc() &&
       match(Op1, m_c_FMul(m_Specific(Op0), m_Value(Y)))) {
     I.setOperand(0, ConstantFP::get(I.getType(), 1.0));
     I.setOperand(1, Y);
     return &I;
   }
 
+  // X / fabs(X) -> copysign(1.0, X)
+  // fabs(X) / X -> copysign(1.0, X)
+  if (I.hasNoNaNs() && I.hasNoInfs() &&
+      (match(&I,
+             m_FDiv(m_Value(X), m_Intrinsic<Intrinsic::fabs>(m_Deferred(X)))) ||
+       match(&I, m_FDiv(m_Intrinsic<Intrinsic::fabs>(m_Value(X)),
+                        m_Deferred(X))))) {
+    Value *V = Builder.CreateBinaryIntrinsic(
+        Intrinsic::copysign, ConstantFP::get(I.getType(), 1.0), X, &I);
+    return replaceInstUsesWith(I, V);
+  }
   return nullptr;
 }
 
 /// This function implements the transforms common to both integer remainder
 /// instructions (urem and srem). It is called by the visitors to those integer
 /// remainder instructions.
 /// Common integer remainder transforms
 Instruction *InstCombiner::commonIRemTransforms(BinaryOperator &I) {

test/Transforms/InstCombine/fabs-copysign.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -instcombine -S < %s | FileCheck %s
 
 declare double @llvm.fabs.f64(double)
 declare float @llvm.fabs.f32(float)
 declare void @use(double)
 
 declare double @llvm.copysign.f64(double, double)
 declare float @llvm.copysign.f32(float, float)
 
 define double @fabs_copysign(double %x) {
 ; CHECK-LABEL: @fabs_copysign(
-; CHECK-NEXT:    [[F:%.*]] = tail call double @llvm.fabs.f64(double [[X:%.*]])
-; CHECK-NEXT:    [[DIV:%.*]] = fdiv nnan ninf double [[X]], [[F]]
-; CHECK-NEXT:    ret double [[DIV]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan ninf double @llvm.copysign.f64(double 1.000000e+00, double [[X:%.*]])
+; CHECK-NEXT:    ret double [[TMP1]]
 ;
   %f = tail call double @llvm.fabs.f64(double %x)
   %div = fdiv nnan ninf double %x, %f
   ret double %div
 }
 
 define double @fabs_copysign_commuted(double %x) {
 ; CHECK-LABEL: @fabs_copysign_commuted(
-; CHECK-NEXT:    [[F:%.*]] = tail call double @llvm.fabs.f64(double [[X:%.*]])
-; CHECK-NEXT:    [[DIV:%.*]] = fdiv nnan ninf double [[F]], [[X]]
-; CHECK-NEXT:    ret double [[DIV]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan ninf double @llvm.copysign.f64(double 1.000000e+00, double [[X:%.*]])
+; CHECK-NEXT:    ret double [[TMP1]]
 ;
   %f = tail call double @llvm.fabs.f64(double %x)
   %div = fdiv nnan ninf double %f, %x
   ret double %div
 }
 
 
 define float @fabs_copysignf(float %x) {
 ; CHECK-LABEL: @fabs_copysignf(
-; CHECK-NEXT:    [[F:%.*]] = tail call float @llvm.fabs.f32(float [[X:%.*]])
-; CHECK-NEXT:    [[DIV:%.*]] = fdiv nnan ninf float [[X]], [[F]]
-; CHECK-NEXT:    ret float [[DIV]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan ninf float @llvm.copysign.f32(float 1.000000e+00, float [[X:%.*]])
+; CHECK-NEXT:    ret float [[TMP1]]
 ;
   %f = tail call float @llvm.fabs.f32(float %x)
   %div = fdiv nnan ninf float %x, %f
   ret float %div
 }
 
 define double @fabs_copysign_use(double %x) {
 ; CHECK-LABEL: @fabs_copysign_use(
 ; CHECK-NEXT:    [[F:%.*]] = tail call double @llvm.fabs.f64(double [[X:%.*]])
 ; CHECK-NEXT:    call void @use(double [[F]])
-; CHECK-NEXT:    [[DIV:%.*]] = fdiv nnan ninf double [[X]], [[F]]
-; CHECK-NEXT:    ret double [[DIV]]
+; CHECK-NEXT:    [[TMP1:%.*]] = call nnan ninf double @llvm.copysign.f64(double 1.000000e+00, double [[X]])
+; CHECK-NEXT:    ret double [[TMP1]]
 ;
   %f = tail call double @llvm.fabs.f64(double %x)
   call void @use(double %f)
   %div = fdiv nnan ninf double %x, %f
   ret double %div
 }
 
 ; Negative tests