[llvm-commits] PATCH: Teach the x86 backend to fold mask & shift patterns into an addressing mode computation

[Duncan Sands]

Hi Chandler,

> --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> @@ -4237,6 +4237,17 @@ SDValue DAGCombiner::visitZERO_EXTEND(SDNode *N) {
>        return SDValue(N, 0);   // Return N so it doesn't get rechecked!
>      }
>
> +    // fold (zext (truncate (zextload x))) -> (zext (zextload x))
> +    if (LoadSDNode *Load = dyn_cast<LoadSDNode>(N0.getOperand(0).getNode())) {
> +      EVT TruncVT = N0.getValueType(), MemVT = Load->getMemoryVT();
> +      // This is safe as long as the truncate doesn't truncate any of the bits
> +      // loaded from memory and we zero extended the rest of the bits.
> +      if (ISD::isZEXTLoad(Load) && TruncVT.bitsGE(MemVT) && 0) {
> +        return DAG.getNode(ISD::ZERO_EXTEND, N->getDebugLoc(), VT,
> +                           N0.getOperand(0));
> +      }
> +    }

this is a special case of a much more general transform: zext(trunc(x)) ->
zext(x) if the bits dropped by the trunc are known zero [in full generality
the result of the transform might be x if the zext is just restoring the
bits truncated, or trunc(x) (to a different size) if the zext does not
restore all of the truncated bits].  How about implementing the general
transform instead?

> --- a/lib/Target/X86/X86ISelDAGToDAG.cpp
> +++ b/lib/Target/X86/X86ISelDAGToDAG.cpp
> @@ -814,6 +814,106 @@ bool X86DAGToDAGISel::MatchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
>      break;
>      }
>
> +  case ISD::SRL: {
> +    // Try some heroics to detect shifts of masked values where the mask can be
> +    // replaced by extending the shift and undoing that in the addressing mode
> +    // scale. Patterns such as (shl (srl x, c1), c2) are canonicalized into
> +    // (and (srl x, SHIFT), MASK) by DAGCombines that don't know the shl can be
> +    // done in the addressing mode.

How about an explicit example of what you want to achieve?

> +
> +    // Scale must not be used already.
> +    if (AM.IndexReg.getNode() != 0 || AM.Scale != 1) break;
> +
> +    if (N.getNumOperands() != 2) break;

Can this check actually ever fire?  Also, might this be loading a vector value?

> +    ConstantSDNode *ShiftAmtNode = dyn_cast<ConstantSDNode>(N.getOperand(1));
> +    if (!ShiftAmtNode) break;
> +    SDValue And = N.getOperand(0);
> +    if (And.getOpcode() != ISD::AND || And.getNumOperands() != 2) break;

Can an "and" really have more than two operands?

> +    ConstantSDNode *MaskNode = dyn_cast<ConstantSDNode>(And.getOperand(1));
> +    if (!MaskNode) break;
> +    SDValue X = And.getOperand(0);
> +
> +    // We only handle up to 64-bit values here for simplicity.
> +    if (X.getValueSizeInBits() > 64) break;
> +
> +    // The number of bits that would have to be shifted left is the number of
> +    // zero bits in the mask *after* it is shifted right.

Are you sure about that?  shl(shr x, N),M should result in M bits being masked.
Thus I would have expected AMShiftAmt below to be MaskTZ (assuming AMShiftAmt
is the amount the zapped shl would have shifted by, which is what your comment
suggested to me; how about more explicit variable names or comments).

> +    uint64_t Mask = MaskNode->getZExtValue();
> +    unsigned ShiftAmt = ShiftAmtNode->getZExtValue();
> +    unsigned MaskLZ = CountLeadingZeros_64(Mask);
> +    unsigned MaskTZ = CountTrailingZeros_64(Mask);
> +    int AMShiftAmt = MaskTZ - ShiftAmt;
> +
> +    // There is nothing we can do here unless the mask is removing some bits.
> +    // Also, the addressing mode can only represent shifts of 1, 2, or 3 bits.
> +    if (AMShiftAmt <= 0 || AMShiftAmt > 3) break;
> +
> +    // We also need to ensure that mask is a continuous run of bits.
> +    if (CountTrailingOnes_64(Mask >> MaskTZ) + MaskTZ + MaskLZ != 64) break;

Is this the best way to check this?

> +
> +    // Scale the leading zero count down based on the actual size of the value.
> +    MaskLZ -= 64 - X.getValueSizeInBits();

I got bored at this point and stopped thinking about this.

Ciao, Duncan.