[InstCombine] transform masking off of an FP sign bit into a fabs() intrinsic call (PR24886)

[spatel]

Summary

This is a partial fix for PR24886:
https://llvm.org/bugs/show_bug.cgi?id=24886

Without this IR transform, the backend (x86 at least) was producing inefficient code.

This patch is making 2 assumptions:

1. The canonical form of a fabs() operation is, in fact, the LLVM fabs() intrinsic.
2. The high bit of an FP value is always the sign bit; as noted in the bug report, this isn't specified by the LangRef.

[majnemer]

If we will canonicalize away from bitmath, can we please teach computeKnownBitsFromOperator how to reason about llvm.fabs? Otherwise, this might be a pessimization.

[spatel]

In D13076#251181, @majnemer wrote:

If we will canonicalize away from bitmath, can we please teach computeKnownBitsFromOperator how to reason about llvm.fabs? Otherwise, this might be a pessimization.

Sure, that seems fair. I see we already have that kind of logic for a few other intrinsics, so fabs should slide right in there.

[sanjoy]

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp

1471 ↗

(On Diff #35429)

I think this comment should now be sunk to the inner if.

1507 ↗

(On Diff #35429)

Is this optimization valid if the cast is actually an fptosi or fptoui? They're also CastInsts.

[spatel]

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp

1471 ↗

(On Diff #35429)

Yes - fixed.

1507 ↗

(On Diff #35429)

No, it should only fire on a bitcast.
Thanks for catching that! I've added an additional check and test case to make sure we're not altering an fptoui.

[spatel]

Patch updated:

1. Moved comment that became displaced.
2. Added check and test case to make sure that we're only transforming a bitcast (not a fptoui).

[spatel]

In D13076#251834, @spatel wrote:

In D13076#251181, @majnemer wrote:

If we will canonicalize away from bitmath, can we please teach computeKnownBitsFromOperator how to reason about llvm.fabs? Otherwise, this might be a pessimization.

Sure, that seems fair. I see we already have that kind of logic for a few other intrinsics, so fabs should slide right in there.

Except that computeKnownBits and friends make assumptions about only operating on integers and scalars. There's no quick fix AFAICT.
See also: https://llvm.org/bugs/show_bug.cgi?id=24942

[spatel]

Ping.

The requested change to computeKnownBits (just scalars for now) is in D13222.

[sanjoy]

I have no more comments to make, so I'll wait for @majnemer take a look.

[spatel]

Ping * 2.

[majnemer]

LGTM

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

Instruction *InstCombiner::visitAnd(BinaryOperator &I) {
 
   // If and'ing two fcmp, try combine them into one.
   if (FCmpInst *LHS = dyn_cast<FCmpInst>(I.getOperand(0)))
     if (FCmpInst *RHS = dyn_cast<FCmpInst>(I.getOperand(1)))
       if (Value *Res = FoldAndOfFCmps(LHS, RHS))
         return ReplaceInstUsesWith(I, Res);
 
 
+  if (CastInst *Op0C = dyn_cast<CastInst>(Op0)) {
+    Value *Op0COp = Op0C->getOperand(0);
+    Type *SrcTy = Op0COp->getType();
     // fold (and (cast A), (cast B)) -> (cast (and A, B))
-  if (CastInst *Op0C = dyn_cast<CastInst>(Op0))
     if (CastInst *Op1C = dyn_cast<CastInst>(Op1)) {
-      Type *SrcTy = Op0C->getOperand(0)->getType();
       if (Op0C->getOpcode() == Op1C->getOpcode() && // same cast kind ?
           SrcTy == Op1C->getOperand(0)->getType() &&
           SrcTy->isIntOrIntVectorTy()) {
-        Value *Op0COp = Op0C->getOperand(0), *Op1COp = Op1C->getOperand(0);
+        Value *Op1COp = Op1C->getOperand(0);
 
         // Only do this if the casts both really cause code to be generated.
         if (ShouldOptimizeCast(Op0C->getOpcode(), Op0COp, I.getType()) &&
             ShouldOptimizeCast(Op1C->getOpcode(), Op1COp, I.getType())) {
           Value *NewOp = Builder->CreateAnd(Op0COp, Op1COp, I.getName());
           return CastInst::Create(Op0C->getOpcode(), NewOp, I.getType());
         }
 
         // If this is and(cast(icmp), cast(icmp)), try to fold this even if the
         // cast is otherwise not optimizable.  This happens for vector sexts.
         if (ICmpInst *RHS = dyn_cast<ICmpInst>(Op1COp))
           if (ICmpInst *LHS = dyn_cast<ICmpInst>(Op0COp))
             if (Value *Res = FoldAndOfICmps(LHS, RHS))
               return CastInst::Create(Op0C->getOpcode(), Res, I.getType());
 
         // If this is and(cast(fcmp), cast(fcmp)), try to fold this even if the
         // cast is otherwise not optimizable.  This happens for vector sexts.
         if (FCmpInst *RHS = dyn_cast<FCmpInst>(Op1COp))
           if (FCmpInst *LHS = dyn_cast<FCmpInst>(Op0COp))
             if (Value *Res = FoldAndOfFCmps(LHS, RHS))
               return CastInst::Create(Op0C->getOpcode(), Res, I.getType());
       }
     }
 
+    // If we are masking off the sign bit of a floating-point value, convert
+    // this to the canonical fabs intrinsic call and cast back to integer.
+    // The backend should know how to optimize fabs().
+    // TODO: This transform should also apply to vectors.
+    ConstantInt *CI;
+    if (isa<BitCastInst>(Op0C) && SrcTy->isFloatingPointTy() &&
+        match(Op1, m_ConstantInt(CI)) && CI->isMaxValue(true)) {
+      Module *M = I.getParent()->getParent()->getParent();
+      Function *Fabs = Intrinsic::getDeclaration(M, Intrinsic::fabs, SrcTy);
+      Value *Call = Builder->CreateCall(Fabs, Op0COp, "fabs");
+      return CastInst::CreateBitOrPointerCast(Call, I.getType());
+    }
+  }
+
   {
     Value *X = nullptr;
     bool OpsSwapped = false;
     // Canonicalize SExt or Not to the LHS
     if (match(Op1, m_SExt(m_Value())) ||
         match(Op1, m_Not(m_Value()))) {
       std::swap(Op0, Op1);
       OpsSwapped = true;

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

 ; CHECK-NOT: %add = add i64 %sub, %and
 ; CHECK-NEXT: %add = sub i64 %and, %x
 ; CHECK-NEXT: ret i64 %add
   %sub = sub nsw i64 0, %x
   %and = and i64 %sub, 1
   %add = add i64 %sub, %and
   ret i64 %add
 }
+
+define i64 @fabs_double(double %x) {
+; CHECK-LABEL: @fabs_double(
+; CHECK-NEXT:  %fabs = call double @llvm.fabs.f64(double %x)
+; CHECK-NEXT:  %and = bitcast double %fabs to i64
+; CHECK-NEXT:  ret i64 %and
+  %bc = bitcast double %x to i64
+  %and = and i64 %bc, 9223372036854775807
+  ret i64 %and
+}
+
+define i64 @fabs_double_swap(double %x) {
+; CHECK-LABEL: @fabs_double_swap(
+; CHECK-NEXT:  %fabs = call double @llvm.fabs.f64(double %x)
+; CHECK-NEXT:  %and = bitcast double %fabs to i64
+; CHECK-NEXT:  ret i64 %and
+  %bc = bitcast double %x to i64
+  %and = and i64 9223372036854775807, %bc
+  ret i64 %and
+}
+
+define i32 @fabs_float(float %x) {
+; CHECK-LABEL: @fabs_float(
+; CHECK-NEXT:  %fabs = call float @llvm.fabs.f32(float %x)
+; CHECK-NEXT:  %and = bitcast float %fabs to i32
+; CHECK-NEXT:  ret i32 %and
+  %bc = bitcast float %x to i32
+  %and = and i32 %bc, 2147483647
+  ret i32 %and
+}
+
+; Make sure that only a bitcast is transformed.
+
+define i64 @fabs_double_not_bitcast(double %x) {
+; CHECK-LABEL: @fabs_double_not_bitcast(
+; CHECK-NEXT:  %bc = fptoui double %x to i64
+; CHECK-NEXT:  %and = and i64 %bc, 9223372036854775807
+; CHECK-NEXT:  ret i64 %and
+  %bc = fptoui double %x to i64
+  %and = and i64 %bc, 9223372036854775807
+  ret i64 %and
+}
+