<div class="gmail_quote">I've attached an updated patch. This addresses your comments as well as a bunch of comment Owen made to me on IRC. There was some overlap. It also includes an extra test case that helped me catch some cases it wasn't firing, and it factors all of the logic out into a helper function. This is used to apply the logic whether the AND or the SRL node come first in the DAG, I've found inputs with it both ways around (hence the extra test case) and the transform itself is easily generalized to either ordering.</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">Owen expressed some concerns about whether this is doing the correct thing WRT the topological sort of the DAG, and I'm now fairly confident in this part. The code inserts the new nodes, in the correct sequence, each one before the input node 'N', so they will be ordered correctly for the remaining ISel progression. The contract of ISel is slightly violated by assigning them all the same node ID number, so that the IDs are no longer unique, but there are several other transforms in the same section (x86 address mode matching) that do the same thing, so this appears to be fine in practice. My suspicion is that the IDs merely need to form a SWO or some such, and that at least is satisfied. I could write code that would re-number all subsequent nodes in order to have unique numbers again, but I don't think it should be part of this patch considering the existing precedent.</div>
<div class="gmail_quote"><br></div><div class="gmail_quote">I'm also starting more extensive testing of this code to make sure it doesn't silently miscompile anything.</div><div class="gmail_quote"><br></div><div class="gmail_quote">
On Sun, Jan 8, 2012 at 6:01 AM, Duncan Sands <span dir="ltr"><<a href="mailto:baldrick@free.fr">baldrick@free.fr</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Chandler,<br>
<br>
> --- a/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br>
> +++ b/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br>
> @@ -4237,6 +4237,17 @@ SDValue DAGCombiner::visitZERO_EXTEND(SDNode *N) {<br>
> return SDValue(N, 0); // Return N so it doesn't get rechecked!<br>
> }<br>
><br>
> + // fold (zext (truncate (zextload x))) -> (zext (zextload x))<br>
> + if (LoadSDNode *Load = dyn_cast<LoadSDNode>(N0.getOperand(0).getNode())) {<br>
> + EVT TruncVT = N0.getValueType(), MemVT = Load->getMemoryVT();<br>
> + // This is safe as long as the truncate doesn't truncate any of the bits<br>
> + // loaded from memory and we zero extended the rest of the bits.<br>
> + if (ISD::isZEXTLoad(Load) && TruncVT.bitsGE(MemVT) && 0) {<br>
> + return DAG.getNode(ISD::ZERO_EXTEND, N->getDebugLoc(), VT,<br>
> + N0.getOperand(0));<br>
> + }<br>
> + }<br>
<br>
this is a special case of a much more general transform: zext(trunc(x)) -><br>
zext(x) if the bits dropped by the trunc are known zero [in full generality<br>
the result of the transform might be x if the zext is just restoring the<br>
bits truncated, or trunc(x) (to a different size) if the zext does not<br>
restore all of the truncated bits]. How about implementing the general<br>
transform instead?<br></blockquote><div><br></div><div>Done.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> --- a/lib/Target/X86/X86ISelDAGToDAG.cpp<br>
> +++ b/lib/Target/X86/X86ISelDAGToDAG.cpp<br>
> @@ -814,6 +814,106 @@ bool X86DAGToDAGISel::MatchAddressRecursively(SDValue N, X86ISelAddressMode &AM,<br>
> break;<br>
> }<br>
><br>
> + case ISD::SRL: {<br>
> + // Try some heroics to detect shifts of masked values where the mask can be<br>
> + // replaced by extending the shift and undoing that in the addressing mode<br>
> + // scale. Patterns such as (shl (srl x, c1), c2) are canonicalized into<br>
> + // (and (srl x, SHIFT), MASK) by DAGCombines that don't know the shl can be<br>
> + // done in the addressing mode.<br>
<br>
How about an explicit example of what you want to achieve?<br></blockquote><div><br></div><div>Added, although its a bit weird to have here instead of in the testcase. Still, maybe it helps as I agree this is a bit of a weird transform.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> + // Scale must not be used already.<br>
> + if (AM.IndexReg.getNode() != 0 || AM.Scale != 1) break;<br>
> +<br>
> + if (N.getNumOperands() != 2) break;<br>
<br>
Can this check actually ever fire? Also, might this be loading a vector value?<br></blockquote><div><br></div><div>The check was dead, but I don't understand the comment about vector values? This may be about to load a vector, but the input shouldn't be. None of the existing code worries about vectors, likely because legalize has already precluded them from being inputs to this chain?</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + ConstantSDNode *ShiftAmtNode = dyn_cast<ConstantSDNode>(N.getOperand(1));<br>
> + if (!ShiftAmtNode) break;<br>
> + SDValue And = N.getOperand(0);<br>
> + if (And.getOpcode() != ISD::AND || And.getNumOperands() != 2) break;<br>
<br>
Can an "and" really have more than two operands?<br></blockquote><div><br></div><div>Nope, nuked.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + ConstantSDNode *MaskNode = dyn_cast<ConstantSDNode>(And.getOperand(1));<br>
> + if (!MaskNode) break;<br>
> + SDValue X = And.getOperand(0);<br>
> +<br>
> + // We only handle up to 64-bit values here for simplicity.<br>
> + if (X.getValueSizeInBits() > 64) break;<br>
> +<br>
> + // The number of bits that would have to be shifted left is the number of<br>
> + // zero bits in the mask *after* it is shifted right.<br>
<br>
Are you sure about that? shl(shr x, N),M should result in M bits being masked.<br>
Thus I would have expected AMShiftAmt below to be MaskTZ (assuming AMShiftAmt<br>
is the amount the zapped shl would have shifted by, which is what your comment<br>
suggested to me; how about more explicit variable names or comments).<br></blockquote><div><br></div><div>Sorry, this comment was stale, and highly misleading, as it dated from an earlier version of th ecode.. I've removed it as there is a much better comment now directly attached to AMShiftAmt.</div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + uint64_t Mask = MaskNode->getZExtValue();<br>
> + unsigned ShiftAmt = ShiftAmtNode->getZExtValue();<br>
> + unsigned MaskLZ = CountLeadingZeros_64(Mask);<br>
> + unsigned MaskTZ = CountTrailingZeros_64(Mask);<br>
> + int AMShiftAmt = MaskTZ - ShiftAmt;<br>
> +<br>
> + // There is nothing we can do here unless the mask is removing some bits.<br>
> + // Also, the addressing mode can only represent shifts of 1, 2, or 3 bits.<br>
> + if (AMShiftAmt <= 0 || AMShiftAmt > 3) break;<br>
> +<br>
> + // We also need to ensure that mask is a continuous run of bits.<br>
> + if (CountTrailingOnes_64(Mask >> MaskTZ) + MaskTZ + MaskLZ != 64) break;<br>
<br>
Is this the best way to check this?<br></blockquote><div><br></div><div>Other code uses the same pattern to check this... If there is a better way, I'd love to know it? It seems... oddly elegant.</div></div>