[llvm-commits] [llvm] r122090 - in /llvm/trunk: lib/CodeGen/SelectionDAG/DAGCombiner.cpp test/CodeGen/X86/x86_64-mul-by-const.ll

[Chris Lattner]

On Dec 17, 2010, at 1:45 PM, Dale Johannesen wrote:

> Author: johannes
> Date: Fri Dec 17 15:45:49 2010
> New Revision: 122090
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=122090&view=rev
> Log:
> Add a transform to DAG Combiner.  This improves the
> code for the case where 32-bit divide by constant is
> turned into 64-bit multiply by constant.  8771012.

Very nice Dale, thanks for fixing this.  Some minor comments:

> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Fri Dec 17 15:45:49 2010
> @@ -3171,6 +3171,26 @@
>                        DAG.getConstant(c1 + c2, N1.getValueType()));
>   }
> 
> +  // fold (srl (trunc (srl x, c1)), c2) -> 0 or (trunc (srl x, (add c1, c2)))
> +  // This is only valid if the OpSizeInBits + c1 = size of inner shift

Please end the comment with a ".".  Do we already do this for arithmetic shifts, and shift left (with extension instead of trunc) as well?

> +  if (N1C && N0.getOpcode() == ISD::TRUNCATE &&
> +      N0.getOperand(0).getOpcode() == ISD::SRL &&
> +      N0.getOperand(0)->getOperand(1).getOpcode() == ISD::Constant) {

Please use isa<ConstantSDNode>(N0.getOperand(0)->getOperand(1)) which makes the cast<> more obviously right.

> +    uint64_t c1 = 
> +      cast<ConstantSDNode>(N0.getOperand(0)->getOperand(1))->getZExtValue();
> +    uint64_t c2 = N1C->getZExtValue();

This can break if the value is > i64.  Instead of doing this on getZExtValue(), I'd suggest doing the math on APInt's.

> +    EVT InnerShiftVT = N0.getOperand(0)->getOperand(1).getValueType();
> +    uint64_t InnerShiftSize = InnerShiftVT.getScalarType().getSizeInBits();
> +    if (c1 + OpSizeInBits == InnerShiftSize) {
> +      if (c1 + c2 >= InnerShiftSize)
> +        return DAG.getConstant(0, VT);
> +      return DAG.getNode(ISD::TRUNCATE, N0->getDebugLoc(), VT,

Please add a short comment explaining what you're doing here.  What is the "c1 + OpSizeInBits == InnerShiftSize" check doing?

> +; RUN: llc < %s -mtriple=x86_64-apple-darwin | FileCheck %s
> +; Formerly there were two shifts.  8771012.

Please add a rdar:// prefix so that grep will pick it up, thanks!

-Chris