transform obscured FP sign bit ops into a fabs/fneg using TLI hook

[spatel]

Summary

This is effectively a revert of:
http://reviews.llvm.org/rL249702
and a reimplementation as a DAG combine for targets that have IEEE754-compliant fabs/fneg instructions.

This is intended to resolve the objections raised on the dev list:
http://lists.llvm.org/pipermail/llvm-dev/2016-April/098154.html
and:
https://llvm.org/bugs/show_bug.cgi?id=24886#c4

In the interest of patch minimalism, I've only partly enabled AArch64. PowerPC, MIPS, x86 and others can enable later.

[RKSimon]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

7465

Can isConstOrConstSplat be used here to allow this combine to work on vectors as well?

[spatel]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

7465

Yes, I thought about adding that, but my guess was that it wouldn't have much impact since vector bitcasts are usually free and hopefully removed elsewhere.

Also, the InstCombine doesn't handle vectors, so this was the 1-to-1 replacement. I can add a TODO comment for vectors though.

[escha]

Our target has a legal FABS (it is in fact, free, as it's a modifier). But it's implemented as FADD DST, SRC.ABS, -0.0, which may modify bits other than the top bit.

[spatel]

In D19391#409039, @escha wrote:

Our target has a legal FABS (it is in fact, free, as it's a modifier). But it's implemented as FADD DST, SRC.ABS, -0.0, which may modify bits other than the top bit.

Ok, I've misunderstood the definition of 'legal' then. I thought that using FADD in place of FABS would be considered "Custom" rather than "Legal". This means we need a new target hook. Any ideas about what that hook would look like?

[spatel]

Note also that the SDNode definitions for 'FABS' and friends is unfortunately vague:
"Perform various unary floating point operations. These are inspired by libm."

[escha]

As far as I know -- "Custom" means something is not legal, but has a custom lowering. It has nothing to do with how it's implemented on the target; it just means that the target has a custom legalization hook (as opposed to Expand or such, which means it doesn't have any custom hook).

We keep FABS legal until the end (much like AMDGPU or similar) -- it's emitted either by folding a modifier onto an existing instruction, or if there's no way to do that, by emitting a "no-op" add. But this happens in Select().

[hfinkel]

In D19391#409050, @spatel wrote:

In D19391#409039, @escha wrote:

Our target has a legal FABS (it is in fact, free, as it's a modifier). But it's implemented as FADD DST, SRC.ABS, -0.0, which may modify bits other than the top bit.

If that's observable, then we'll need a TLI hook to control this. The hook probably also need to check the type (I imagine this might not be correct for ppc_fp128, for example).

Ok, I've misunderstood the definition of 'legal' then. I thought that using FADD in place of FABS would be considered "Custom" rather than "Legal". This means we need a new target hook. Any ideas about what that hook would look like?

Yes, legal means "I know this will pattern match". The pattern, however, could do anything. You should check for legal-or-custom, however. The practice of taking legal to mean cheap and custom to mean more expensive is pretty broken in many cases.

[spatel]

Patch updated:

1. Instead of checking legality of fabs/fneg, provide a TLI hook that can predicate as needed.
2. Instead of handling fabs alone, implement a fabs/fneg helper function leaving vectors and fnabs as TODO items.

[escha]

This is a nice solution -- instead of worrying too much about LLVM float semantic definitions, just let the target say how it implements things.

[spatel]

Ping.

[spatel]

Ping * 2.

Patch updated: rebased and fixed code comments.

[spatel]

Ping * 3.

[scanon]

Numerically, I'm fine with this. Someone else can address the approach and style, etc.

[spatel]

Ping * 4.

[majnemer]

IIRC, I asked you to add support for fabs to computeKnownBits when you added this canonicalization.

Seeing as how the canonicalization is removed, I think we should undo this aspect of computeKnownBits.

[spatel]

In D19391#447144, @majnemer wrote:

IIRC, I asked you to add support for fabs to computeKnownBits when you added this canonicalization.

Yes, that was D13222 / rL249701 .

Seeing as how the canonicalization is removed, I think we should undo this aspect of computeKnownBits.

What is the advantage of removing the analysis? It is still true that an llvm fabs() intrinsic clears the sign bit in all cases, right?

[majnemer]

In D19391#447164, @spatel wrote:

In D19391#447144, @majnemer wrote:

IIRC, I asked you to add support for fabs to computeKnownBits when you added this canonicalization.

Yes, that was D13222 / rL249701 .

Seeing as how the canonicalization is removed, I think we should undo this aspect of computeKnownBits.

What is the advantage of removing the analysis? It is still true that an llvm fabs() intrinsic clears the sign bit in all cases, right?

I'm not sure if what we were allowed to assume about the bitpattern of float/double from the middle end.
If it's fine, then this LGTM without touching ValueTracking.

[spatel]

In D19391#447172, @majnemer wrote:

I'm not sure if what we were allowed to assume about the bitpattern of float/double from the middle end.
If it's fine, then this LGTM without touching ValueTracking.

Ah, right - the LangRef doesn't specify anything about the FP formats. Let's be safe then. I'll update this patch to revert rL249701 .

[spatel]

Patch updated:
Revert r249701 ([ValueTracking] teach computeKnownBits that a fabs() clears sign bits).
The LangRef mentions IEEE format for NaN/Inf in the IR, but I'm not sure if that allows us to deduce the position of the sign bit in IR FP formats. To be safe, we'll just remove the fabs() ValueTracking logic for now.

[majnemer]

LGTM

include/llvm/Target/TargetLowering.h

virtual bool isCheapToSpeculateCttz() const {
     return false;
   }
 
   /// \brief Return true if it is cheap to speculate a call to intrinsic ctlz.
   virtual bool isCheapToSpeculateCtlz() const {
     return false;
   }
 
+  /// Return true if it is safe to transform an integer-domain bitwise operation
+  /// into the equivalent floating-point operation. This should be set to true
+  /// if the target has IEEE-754-compliant fabs/fneg operations for the input
+  /// type.
+  virtual bool hasBitPreservingFPLogic(EVT VT) const {
+    return false;
+  }
+
   /// \brief Return if the target supports combining a
   /// chain like:
   /// \code
   ///   %andResult = and %val1, #imm-with-one-bit-set;
   ///   %icmpResult = icmp %andResult, 0
   ///   br i1 %icmpResult, label %dest1, label %dest2
   /// \endcode
   /// into a single machine instruction of a form like:

lib/Analysis/ValueTracking.cpp

case Instruction::Trunc: {
     KnownOne = KnownOne.zextOrTrunc(BitWidth);
     // Any top bits are known to be zero.
     if (BitWidth > SrcBitWidth)
       KnownZero |= APInt::getHighBitsSet(BitWidth, BitWidth - SrcBitWidth);
     break;
   }
   case Instruction::BitCast: {
     Type *SrcTy = I->getOperand(0)->getType();
-    if ((SrcTy->isIntegerTy() || SrcTy->isPointerTy() ||
-         SrcTy->isFloatingPointTy()) &&
+    if ((SrcTy->isIntegerTy() || SrcTy->isPointerTy()) &&
         // TODO: For now, not handling conversions like:
         // (bitcast i64 %x to <2 x i32>)
         !I->getType()->isVectorTy()) {
       computeKnownBits(I->getOperand(0), KnownZero, KnownOne, Depth + 1, Q);
       break;
     }
     break;
   }
if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I)) {
           APInt(BitWidth, BitsPossiblySet).countLeadingZeros();
         assert(LeadingZeros <= BitWidth);
         KnownZero |= APInt::getHighBitsSet(BitWidth, LeadingZeros);
         KnownOne &= ~KnownZero;
         // TODO: we could bound KnownOne using the lower bound on the number
         // of bits which might be set provided by popcnt KnownOne2.
         break;
       }
-      case Intrinsic::fabs: {
-        Type *Ty = II->getType();
-        APInt SignBit = APInt::getSignBit(Ty->getScalarSizeInBits());
-        KnownZero |= APInt::getSplat(Ty->getPrimitiveSizeInBits(), SignBit);
-        break;
-      }
       case Intrinsic::x86_sse42_crc32_64_64:
         KnownZero |= APInt::getHighBitsSet(64, 32);
         break;
       }
     }
     break;
   case Instruction::ExtractValue:
     if (IntrinsicInst *II = dyn_cast<IntrinsicInst>(I->getOperand(0))) {

 /// for all of the elements in the vector.
 void computeKnownBits(Value *V, APInt &KnownZero, APInt &KnownOne,
                       unsigned Depth, const Query &Q) {
   assert(V && "No Value?");
   assert(Depth <= MaxDepth && "Limit Search Depth");
   unsigned BitWidth = KnownZero.getBitWidth();
 
   assert((V->getType()->isIntOrIntVectorTy() ||
-          V->getType()->isFPOrFPVectorTy() ||
           V->getType()->getScalarType()->isPointerTy()) &&
-         "Not integer, floating point, or pointer type!");
+         "Not integer or pointer type!");
   assert((Q.DL.getTypeSizeInBits(V->getType()->getScalarType()) == BitWidth) &&
          (!V->getType()->isIntOrIntVectorTy() ||
           V->getType()->getScalarSizeInBits() == BitWidth) &&
          KnownZero.getBitWidth() == BitWidth &&
          KnownOne.getBitWidth() == BitWidth &&
          "V, KnownOne and KnownZero should have same BitWidth");
 
   if (ConstantInt *CI = dyn_cast<ConstantInt>(V)) {

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

 }
 
 static unsigned getPPCf128HiElementSelector(const SelectionDAG &DAG) {
   // On little-endian machines, bitcasting from ppcf128 to i128 does swap the Hi
   // and Lo parts; on big-endian machines it doesn't.
   return DAG.getDataLayout().isBigEndian() ? 1 : 0;
 }
 
+static SDValue foldBitcastedFPLogic(SDNode *N, SelectionDAG &DAG,
+                                    const TargetLowering &TLI) {
+  // If this is not a bitcast to an FP type or if the target doesn't have
+  // IEEE754-compliant FP logic, we're done.
+  EVT VT = N->getValueType(0);
+  if (!VT.isFloatingPoint() || !TLI.hasBitPreservingFPLogic(VT))
+    return SDValue();
+
+  // TODO: Use splat values for the constant-checking below and remove this
+  // restriction.
+  SDValue N0 = N->getOperand(0);
+  EVT SourceVT = N0.getValueType();
+  if (SourceVT.isVector())
+    return SDValue();
+
+  unsigned FPOpcode;
+  APInt SignMask;
+  switch (N0.getOpcode()) {
+  case ISD::AND:
+    FPOpcode = ISD::FABS;
+    SignMask = ~APInt::getSignBit(SourceVT.getSizeInBits());
+    break;
+  case ISD::XOR:
+    FPOpcode = ISD::FNEG;
+    SignMask = APInt::getSignBit(SourceVT.getSizeInBits());
+    break;
+  // TODO: ISD::OR --> ISD::FNABS?
+  default:
+    return SDValue();
+  }
+
+  // Fold (bitcast int (and (bitcast fp X to int), 0x7fff...) to fp) -> fabs X
+  // Fold (bitcast int (xor (bitcast fp X to int), 0x8000...) to fp) -> fneg X
+  SDValue LogicOp0 = N0.getOperand(0);
+  ConstantSDNode *LogicOp1 = dyn_cast<ConstantSDNode>(N0.getOperand(1));
+  if (LogicOp1 && LogicOp1->getAPIntValue() == SignMask &&
+      LogicOp0.getOpcode() == ISD::BITCAST &&
+      LogicOp0->getOperand(0).getValueType() == VT)
+    return DAG.getNode(FPOpcode, SDLoc(N), VT, LogicOp0->getOperand(0));
+
+  return SDValue();
+}
+
 SDValue DAGCombiner::visitBITCAST(SDNode *N) {
   SDValue N0 = N->getOperand(0);
   EVT VT = N->getValueType(0);
 
   // If the input is a BUILD_VECTOR with all constant elements, fold this now.
   // Only do this before legalize, since afterward the target may be depending
   // on the bitconvert.
   // First check to see if this is all constant.
if (TLI.allowsMemoryAccess(*DAG.getContext(), DAG.getDataLayout(), VT,
                                  LN0->isVolatile(), LN0->isNonTemporal(),
                                  LN0->isInvariant(), OrigAlign,
                                  LN0->getAAInfo());
       DAG.ReplaceAllUsesOfValueWith(N0.getValue(1), Load.getValue(1));
       return Load;
     }
   }
 
+  if (SDValue V = foldBitcastedFPLogic(N, DAG, TLI))
+    return V;
+
   // fold (bitconvert (fneg x)) -> (xor (bitconvert x), signbit)
   // fold (bitconvert (fabs x)) -> (and (bitconvert x), (not signbit))
   //
   // For ppc_fp128:
   // fold (bitcast (fneg x)) ->
   //     flipbit = signbit
   //     (xor (bitcast x) (build_pair flipbit, flipbit))
   //
   // fold (bitcast (fabs x)) ->
   //     flipbit = (and (extract_element (bitcast x), 0), signbit)

lib/Target/AArch64/AArch64ISelLowering.h

public:
 
   bool isCheapToSpeculateCttz() const override {
     return true;
   }
 
   bool isCheapToSpeculateCtlz() const override {
     return true;
   }
+
+  bool hasBitPreservingFPLogic(EVT VT) const override {
+    // FIXME: Is this always true? It should be true for vectors at least.
+    return VT == MVT::f32 || VT == MVT::f64;
+  }
+
   bool supportSplitCSR(MachineFunction *MF) const override {
     return MF->getFunction()->getCallingConv() == CallingConv::CXX_FAST_TLS &&
            MF->getFunction()->hasFnAttribute(Attribute::NoUnwind);
   }
   void initializeSplitCSR(MachineBasicBlock *Entry) const override;
   void insertCopiesSplitCSR(
       MachineBasicBlock *Entry,
       const SmallVectorImpl<MachineBasicBlock *> &Exits) const override;

lib/Transforms/InstCombine/InstCombineAndOrXor.cpp

Instruction *InstCombiner::visitAnd(BinaryOperator &I) {
   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 (Instruction *CastedAnd = foldCastedBitwiseLogic(I))
     return CastedAnd;
 
-  if (CastInst *Op0C = dyn_cast<CastInst>(Op0)) {
-    Value *Op0COp = Op0C->getOperand(0);
-    Type *SrcTy = Op0COp->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.getModule();
-      Function *Fabs = Intrinsic::getDeclaration(M, Intrinsic::fabs, SrcTy);
-      Value *Call = Builder->CreateCall(Fabs, Op0COp, "fabs");
-      return CastInst::CreateBitOrPointerCast(Call, I.getType());
-    }
-  }
-
   if (Instruction *Select = foldBoolSextMaskToSelect(I))
     return Select;
 
   return Changed ? &I : nullptr;
 }
 
 /// Given an OR instruction, check to see if this is a bswap idiom. If so,
 /// insert the new intrinsic and return it.

test/CodeGen/AArch64/fcvt-int.ll

 ; CHECK: fmov {{d[0-9]+}}, {{x[0-9]+}}
    ret double %res
 
 }
 
 define double @bitcast_fabs(double %x) {
 ; CHECK-LABEL: bitcast_fabs:
 ; CHECK:       ; BB#0:
-; CHECK-NEXT:    fmov x8, d0
-; CHECK-NEXT:    and x8, x8, #0x7fffffffffffffff
-; CHECK-NEXT:    fmov d0, x8
+; CHECK-NEXT:    fabs d0, d0
 ; CHECK-NEXT:    ret
 ;
   %bc1 = bitcast double %x to i64
   %and = and i64 %bc1, 9223372036854775807
   %bc2 = bitcast i64 %and to double
   ret double %bc2
 }
 
 define float @bitcast_fneg(float %x) {
 ; CHECK-LABEL: bitcast_fneg:
 ; CHECK:       ; BB#0:
-; CHECK-NEXT:    fmov w8, s0
-; CHECK-NEXT:    eor w8, w8, #0x80000000
-; CHECK-NEXT:    fmov s0, w8
+; CHECK-NEXT:    fneg s0, s0
 ; CHECK-NEXT:    ret
 ;
   %bc1 = bitcast float %x to i32
   %xor = xor i32 %bc1, 2147483648
   %bc2 = bitcast i32 %xor to float
   ret float %bc2
 }
 

test/Transforms/InstCombine/and2.ll

 ; 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
-}
-

test/Transforms/InstCombine/fabs.ll

 ; CHECK-NEXT: ret fp128 %mul
 }
 
 ; Make sure all intrinsic calls are eliminated when the input is known positive.
 
 declare float @llvm.fabs.f32(float)
 declare double @llvm.fabs.f64(double)
 declare fp128 @llvm.fabs.f128(fp128)
-declare <4 x float> @llvm.fabs.v4f32(<4 x float>)
 
 define float @square_fabs_intrinsic_f32(float %x) {
   %mul = fmul float %x, %x
   %fabsf = tail call float @llvm.fabs.f32(float %mul)
   ret float %fabsf
 
 ; CHECK-LABEL: square_fabs_intrinsic_f32(
 ; CHECK-NEXT: %mul = fmul float %x, %x
define float @square_fabs_shrink_call2(float %x) {
   %trunc = fptrunc double %fabs to float
   ret float %trunc
 
 ; CHECK-LABEL: square_fabs_shrink_call2(
 ; CHECK-NEXT: %sq = fmul float %x, %x
 ; CHECK-NEXT: ret float %sq
 }
 
-; A scalar fabs op makes the sign bit zero, so masking off all of the other bits means we can return zero.
-
-define i32 @fabs_value_tracking_f32(float %x) {
-  %call = call float @llvm.fabs.f32(float %x)
-  %bc = bitcast float %call to i32
-  %and = and i32 %bc, 2147483648
-  ret i32 %and
-
-; CHECK-LABEL: fabs_value_tracking_f32(
-; CHECK:       ret i32 0
-}
-
-; TODO: A vector fabs op makes the sign bits zero, so masking off all of the other bits means we can return zero.
-
-define <4 x i32> @fabs_value_tracking_v4f32(<4 x float> %x) {
-  %call = call <4 x float> @llvm.fabs.v4f32(<4 x float> %x)
-  %bc = bitcast <4 x float> %call to <4 x i32>
-  %and = and <4 x i32> %bc, <i32 2147483648, i32 2147483648, i32 2147483648, i32 2147483648>
-  ret <4 x i32> %and
-
-; CHECK-LABEL: fabs_value_tracking_v4f32(
-; CHECK:       ret <4 x i32> %and
-}
-