[llvm-commits] [llvm] r168648 - in /llvm/trunk: include/llvm/Analysis/InstructionSimplify.h lib/Analysis/InstructionSimplify.cpp

[Duncan Sands]

Hi Michael,

On 27/11/12 01:46, Michael Ilseman wrote:
> Author: milseman
> Date: Mon Nov 26 18:46:26 2012
> New Revision: 168648
>
> URL: http://llvm.org/viewvc/llvm-project?rev=168648&view=rev
> Log:
> Fast-math optimization: fold multiply by zero
>
> Added in first optimization using fast-math flags to serve as an example for following optimizations. SimplifyInstruction will now try to optimize an fmul observing its FastMathFlags to see if it can fold multiply by zero when 'nnan' and 'nsz' flags are set.

it's great to see some IR level fast-math optimizations at last!  I did notice a
few things though:

> --- llvm/trunk/lib/Analysis/InstructionSimplify.cpp (original)
> +++ llvm/trunk/lib/Analysis/InstructionSimplify.cpp Mon Nov 26 18:46:26 2012
> @@ -886,6 +886,33 @@
>                              RecursionLimit);
>   }
>
> +/// Given the operands for an FMul, see if we can fold the result
> +static Value *SimplifyFMulInst(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::FMul, CLHS->getType(),
> +                                      Ops, Q.TD, Q.TLI);
> +    }

At this point you know that LHS is constant but RHS isn't.  You can put the
LHS "on the right-hand side" by doing: std::swap(Op0, Op1);
For example, consider this code from SimplifyMulInst:

     // Canonicalize the constant to the RHS.
     std::swap(Op0, Op1);

This is OK because floating point multiplication is always commutative.

> + }
> +
> + // Check for some fast-math optimizations
> + if (FMF.NoNaNs) {
> +   if (FMF.NoSignedZeros) {
> +     // fmul N S 0, x ==> 0
> +     if (match(Op0, m_Zero()))
> +       return Op0;

Thanks to the above canonicalization you don't have to test Op0 here.

> +     if (match(Op1, m_Zero()))

Here m_Zero only matches a float with every bit equal to zero, so it doesn't
match negative zero.  I suggest you add new patterns to PatternMatch.h which
are specific to matching different kinds of floating point values.

> +       return Op1;
> +   }
> + }
> +
> + return 0;
> +}
> +
>   /// SimplifyMulInst - Given operands for a Mul, see if we can
>   /// fold the result.  If not, this returns null.
>   static Value *SimplifyMulInst(Value *Op0, Value *Op1, const Query &Q,
> @@ -2799,6 +2834,10 @@
>                                cast<BinaryOperator>(I)->hasNoUnsignedWrap(),
>                                TD, TLI, DT);
>       break;
> +  case Instruction::FMul:
> +    Result = SimplifyFMulInst(I->getOperand(0), I->getOperand(1),
> +                              I->getFastMathFlags(), TD, TLI, DT);
> +    break;
>     case Instruction::Mul:
>       Result = SimplifyMulInst(I->getOperand(0), I->getOperand(1), TD, TLI, DT);
>       break;

Maybe you should add it to SimplifyBinOp too.  True, it won't do anything (as
there are no fast-math flags available there) but I guess it might do something
in the future and it seems odd not to call it from there.  Or maybe add a
comment in SimplifyBinOp explaining why it isn't called.

Ciao, Duncan.