[llvm-commits] Patch: Floating point optimizations for SimplifyInstruction

[Duncan Sands]

Hi Michael,

On 06/12/12 01:34, Michael Ilseman wrote:
> Here's a second round of patches, incorporating the review feedback Duncan had given me earlier but I had spaced. The biggest difference is an extension to PatternMatch.h for pattern matching ConstantFP values of interest.
>
> Patch1: Pattern matching for more floating point values: -0, +/-0, 1.0, and ConstantFPs
>
> Patch2: Added a slew of SimplifyInstruction floating-point optimizations, many of which take advantage of fast-math flags. Test cases included.
>    fsub X, 0 ==> X
>    fsub 0, (fsub 0, X) ==> X
>    fsub X, -0 ==> X
>      when X is known non-negative-zero, e.g. the fsub is nsz.
>    fsub nnan ninf x, x ==> 0.0
>    fadd X, -0 ==> X
>    fadd X, 0 ==> X
>      when X is known non-negative-zero, e.g. the fadd is nsz.
>    fadd [nnan ninf] X, (fsub [nnan ninf] 0, X) ==> 0
>      where nnan and ninf have to occur at least once somewhere in this expression
>    fmul X, 1.0 ==> X


> --- a/lib/Analysis/InstructionSimplify.cpp
> +++ b/lib/Analysis/InstructionSimplify.cpp
> @@ -887,6 +887,82 @@ Value *llvm::SimplifySubInst(Value *Op0, Value *Op1, bool isNSW, bool isNUW,
>                             RecursionLimit);
>  }
>
> +/// Given operands for an FAdd, see if we can fold the result.  If not, this
> +/// returns null.
> +static Value *SimplifyFAddInst(Value *Op0, Value *Op1, FastMathFlags FMF,
> +                              const Query &Q, unsigned MaxRecurse) {
> +  if (Constant *CLHS = dyn_cast<Constant>(Op0)) {
> +    if (Constant *CRHS = dyn_cast<Constant>(Op1)) {
> +      Constant *Ops[] = { CLHS, CRHS };
> +      return ConstantFoldInstOperands(Instruction::FAdd, CLHS->getType(),
> +                                      Ops, Q.TD, Q.TLI);
> +    }
> +
> +    // Canonicalize the constant to the RHS.
> +    std::swap(Op0, Op1);
> +  }
> +
> +  // fadd X, -0 ==> X
> +  if (match(Op1, m_NegZero()))
> +    return Op0;
> +
> +  // fadd X, 0 ==> X, when we know X is not -0
> +  if (match(Op1, m_Zero()) &&
> +      (FMF.NoSignedZeros || CannotBeNegativeZero(Op0)))
> +    return Op0;
> +
> +  // fadd [nnan ninf] X, (fsub [nnan ninf] 0, X) ==> 0
> +  //   where nnan and ninf have to occur at least once somewhere in this
> +  //   expression
> +  Value *X = 0;

(Bike shed) You don't need to initialize X.  Actually you don't need X at all
(see next comment).

> +  Value *SubOp = 0;
> +  if (match(Op1, m_FSub(m_AnyZero(), m_Value(X))) && X == Op0)

Rather than
   m_Value(X))) && X == Op0
you can do
   m_Specific(Op0)
Same comment applies below too.

> +    SubOp = Op1;
> +  else if (match(Op0, m_FSub(m_AnyZero(), m_Value(X))) && X == Op1)
> +    SubOp = Op0;
> +  if (SubOp) {
> +    Instruction *FSub = cast<Instruction>(SubOp);
> +    if ((FMF.NoNaNs || FSub->hasNoNaNs()) &&
> +        (FMF.NoInfs || FSub->hasNoInfs()))
> +      return Constant::getNullValue(Op0->getType());
> +  }
> +
> +  return 0;
> +}
> +
> +/// Given operands for an FSub, see if we can fold the result.  If not, this
> +/// returns null.
> +static Value *SimplifyFSubInst(Value *Op0, Value *Op1, FastMathFlags FMF,
> +                              const Query &Q, unsigned MaxRecurse) {
> +  if (Constant *CLHS = dyn_cast<Constant>(Op0)) {
> +    if (Constant *CRHS = dyn_cast<Constant>(Op1)) {
> +      Constant *Ops[] = { CLHS, CRHS };
> +      return ConstantFoldInstOperands(Instruction::FSub, CLHS->getType(),
> +                                      Ops, Q.TD, Q.TLI);
> +    }
> +  }
> +
> +  // fsub X, 0 ==> X
> +  if (match(Op1, m_Zero()))
> +    return Op0;
> +
> +  // fsub X, -0 ==> X, when we know X is not -0
> +  if (match(Op1, m_NegZero()) &&
> +      (FMF.NoSignedZeros || CannotBeNegativeZero(Op0)))
> +    return Op0;
> +
> +  // fsub 0, (fsub 0, X) ==> X
> +  Value *X = 0;
> +  if (match(Op0, m_Zero()) && match(Op1, m_FSub(m_Zero(), m_Value(X))))
> +    return X;

Does this also work for -0.0?  With fast-math it should work with mixes
of +0.0 and -0.0 too.


> --- a/include/llvm/Support/PatternMatch.h
> +++ b/include/llvm/Support/PatternMatch.h
> @@ -87,8 +87,36 @@ struct match_zero {
>  /// m_Zero() - Match an arbitrary zero/null constant.  This includes
>  /// zero_initializer for vectors and ConstantPointerNull for pointers.
>  inline match_zero m_Zero() { return match_zero(); }
> -
> -
> +
> +struct match_neg_zero {
> +  template<typename ITy>
> +  bool match(ITy *V) {
> +    if (const Constant *C = dyn_cast<Constant>(V))
> +      return C->isNegativeZeroValue();
> +    return false;
> +  }
> +};
> +
> +/// m_NegZero() - Match an arbitrary zero/null constant.  This includes
> +/// zero_initializer for vectors and ConstantPointerNull for pointers. For
> +/// floating point constants, this will match negative zero but not positive
> +/// zero
> +inline match_neg_zero m_NegZero() { return match_neg_zero(); }
> +
> +struct match_any_zero {
> +  template<typename ITy>
> +  bool match(ITy *V) {
> +    if (const Constant *C = dyn_cast<Constant>(V))
> +      return C->isNullValue() || C->isNegativeZeroValue();
> +    return false;
> +  }
> +};
> +
> +/// m_AnyZero() - Match an arbitrary zero/null constant.  This includes
> +/// zero_initializer for vectors and ConstantPointerNull for pointers. For
> +/// floating point constants, this will match negative zero and positive zero
> +inline match_any_zero m_AnyZero() { return match_any_zero(); }
> +

While I think m_AnyZero is OK as it is, it could probably have been implemented
as some kind of m_Or(m_Zero(...), m_NegZero(...)) thing.

>  struct apint_match {
>    const APInt *&Res;
>    apint_match(const APInt *&R) : Res(R) {}
> @@ -161,7 +189,27 @@ struct cst_pred_ty : public Predicate {
>      return false;
>    }
>  };
> -
> +
> +/// cst_fp_pred_ty - This helper class is used to match scalar and vector
> +/// floating point constants that satisfy a specified predicate.
> +template<typename Predicate>
> +struct cst_fp_pred_ty : public Predicate {
> +  template<typename ITy>
> +  bool match(ITy *V) {
> +    if (const ConstantFP *CFP = dyn_cast<ConstantFP>(V))
> +      return this->isValue(CFP->getValueAPF());
> +    // FIXME: Remove this.

Remove this?

> +    if (const ConstantVector *CV = dyn_cast<ConstantVector>(V))
> +      if (ConstantFP *CFP = dyn_cast_or_null<ConstantFP>(CV->getSplatValue()))
> +        return this->isValue(CFP->getValueAPF());
> +    if (const ConstantDataVector *CV = dyn_cast<ConstantDataVector>(V))
> +      if (ConstantFP *CFP = dyn_cast_or_null<ConstantFP>(CV->getSplatValue()))
> +        return this->isValue(CFP->getValueAPF());
> +    return false;
> +  }
> +};
> +
> +
>  /// api_pred_ty - This helper class is used to match scalar and vector constants
>  /// that satisfy a specified predicate, and bind them to an APInt.
>  template<typename Predicate>
> @@ -198,12 +246,22 @@ struct api_pred_ty : public Predicate {
>
>  struct is_one {
>    bool isValue(const APInt &C) { return C == 1; }
> +  bool isValue(const APFloat &C) {
> +    bool ignored;
> +    APFloat F(1.0);
> +    F.convert(C.getSemantics(), APFloat::rmNearestTiesToEven, &ignored);
> +    return C.bitwiseIsEqual(F);
> +  }

This seems to be doing some heavy operations, is there no cheaper trick?

Otherwise LGTM.

Ciao, Duncan.