[llvm-commits] [llvm] r147936 - in /llvm/trunk: lib/CodeGen/SelectionDAG/DAGCombiner.cpp lib/Target/X86/X86ISelDAGToDAG.cpp test/CodeGen/X86/fold-and-shift.ll

[Chandler Carruth]

On Wed, Jan 11, 2012 at 3:47 AM, Duncan Sands <baldrick at free.fr> wrote:

> Hi Chandler,
>
> > --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
> > +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Wed Jan 11
> 02:41:08 2012
> > @@ -4254,6 +4254,29 @@
> >       return DAG.getNode(ISD::ZERO_EXTEND, N->getDebugLoc(), VT,
> >                          N0.getOperand(0));
> >
> > +  // fold (zext (truncate x)) ->  (zext x) or
> > +  //      (zext (truncate x)) ->  (truncate x)
> > +  // This is valid when the truncated bits of x are already zero.
> > +  // FIXME: We should extend this to work for vectors too.
> > +  if (N0.getOpcode() == ISD::TRUNCATE&&  !VT.isVector()) {
> > +    SDValue Op = N0.getOperand(0);
> > +    APInt TruncatedBits
> > +      = APInt::getBitsSet(Op.getValueSizeInBits(),
> > +                          N0.getValueSizeInBits(),
> > +                          std::min(Op.getValueSizeInBits(),
> > +                                   VT.getSizeInBits()));
> > +    APInt KnownZero, KnownOne;
> > +    DAG.ComputeMaskedBits(Op, TruncatedBits, KnownZero, KnownOne);
> > +    if (TruncatedBits == KnownZero) {
> > +      if (VT.bitsGT(Op.getValueType()))
> > +        return DAG.getNode(ISD::ZERO_EXTEND, N->getDebugLoc(), VT, Op);
> > +      if (VT.bitsLT(Op.getValueType()))
> > +        return DAG.getNode(ISD::TRUNCATE, N->getDebugLoc(), VT, Op);
>
> here you could use getZExtOrTrunc.
>

Will do..


>
> > +
> > +      return Op;
> > +    }
> > +  }
> > +
>
> How about doing the analogous sext and anyext transforms too?
>

Maaaybe... ;] Gonna try to flush out the other state I have  first.


>
> > +static bool FoldMaskAndShiftToScale(SelectionDAG&DAG, SDValue N,
> > +                                    X86ISelAddressMode&AM) {
> > +  // Scale must not be used already.
> > +  if (AM.IndexReg.getNode() != 0 || AM.Scale != 1) return true;
>
> Returning 'true' on failure - ugh!  Maybe this should be mentioned in the
> comment describing the method.
>

I thought I did, but it might have been in a later patch. I'll revisit the
comments and clean them up.

>
> > +    // We looked through an ANY_EXTEND node, insert a ZERO_EXTEND.
> > +    SDValue NewX = DAG.getNode(ISD::ZERO_EXTEND, X.getDebugLoc(), VT,
> X);
> > +    if (NewX.getNode()->getNodeId() == -1 ||
> > +        NewX.getNode()->getNodeId()>  N.getNode()->getNodeId()) {
> > +      DAG.RepositionNode(N.getNode(), NewX.getNode());
> > +      NewX.getNode()->setNodeId(N.getNode()->getNodeId());
>
> What's all the mucking around with node ids etc for?  If you really have
> to do this mysterious stuff, how about adding a little helper for it.


As discussed on IRC, this is to preserve the strict-weak-ordering
relationship between Ids for uses and defs once we're down at this layer.
I've factored all of this out in subsequent patches, as I was able to
factor it out of many different places, not just here.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120111/a408ca61/attachment.html>