[llvm-commits] [llvm] r37851 - /llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

[Dan Gohman]

On Tue, Jul 03, 2007 at 04:43:14PM +0200, Duncan Sands wrote:
> Hi Dan,
> 
> > +  // fold (add x, undef) -> undef
> > +  if (N1.getOpcode() == ISD::UNDEF)
> > +    return N1;
> 
> what if N0 is undef and not N1?

I had thought they'd be canonicalized, but I missed that the dagcombiner
sometimes has non-canonicalized nodes. I'll fix these.

> > -  // If either operand is undef, the result is undef
> > -  if (N0.getOpcode() == ISD::UNDEF || N1.getOpcode() == ISD::UNDEF)
> > -    return DAG.getNode(ISD::UNDEF, VT);
> > -
> 
> Did you move this earlier to make it more efficient, or for another
> reason?

I was looking at the instcombine code, where undefs tend to be handled
toward the beginning. I wasn't trying to micro-optimize dagcombine :).

> > -  // If either operand is undef, the result is undef
> > +  // If either operand of a sub is undef, the result is undef
> >    if (N0.getOpcode() == ISD::UNDEF || N1.getOpcode() == ISD::UNDEF)
> >      return DAG.getNode(ISD::UNDEF, VT);
> 
> If it was more efficient doing this earlier for add, presumably the
> same is true for subtraction.

I guess I can make this more consistent with instcombine too.

> > +  // fold (mul x, undef) -> 0
> > +  if (N1.getOpcode() == ISD::UNDEF)
> > +    return DAG.getConstant(0, VT);
> 
> Yes, this seems better :)  Again, is the undef guaranteed to be
> in N1 and not N0?

Same as add, above.

> > +  // X / undef -> undef
> > +  if (N1.getOpcode() == ISD::UNDEF)
> > +    return N1;
> 
> Here you use that X / 0 is undefined in the usual sense,
> and can take on any value.  Is it undefined?  Also, wouldn't
> it be better to do this before the N0 one, in case you have
> undef/undef ?
> 
> Same comments for udiv, srem and urem.

The primary aim of this mod was to get dagcombine to stop being
over-aggressive. I agree that there does seem to be more room for
optimization, in both instcombine and dagcombine.

> > @@ -1390,10 +1400,6 @@
> >      return DAG.getNode(N0.getOpcode(), VT, ORNode, N0.getOperand(1));
> >    }
> >    
> > -  // If either operand is undef, the result is undef
> > -  if (N0.getOpcode() == ISD::UNDEF || N1.getOpcode() == ISD::UNDEF)
> > -    return DAG.getNode(ISD::UNDEF, VT);
> > -
> >    return SDOperand();
> >  }
> 
> (SimplifyBinOpWithSameOpcodeHands)
> 
> In this case both N0 and N1 must be undef (since by definition they
> have the same opcode here), so returning undef seems ok here.  That
> said, the original "if" was kind of silly since if one opcode is undef
> then so is the other.

This logic was removed in favor of having specific logic in visitAND,
visitOR, and visitXOR. The new code has the same problem as visitADD,
discussed above, so I'll fix that, but then it'll cover the UNDEF op UNDEF
cases as well.

> > +  // fold (or x, undef) -> -1
> > +  if (N1.getOpcode() == ISD::UNDEF)
> > +    return DAG.getConstant(-1, VT);
> 
> Is this the right way to get an all-bits-one value?

I guess DAG.getConstant(~0ULL, VT) is the way its written elsewhere. And I
have to fix it for vectors. I'll work on it.

> > @@ -3040,10 +3051,6 @@
> >    if (isNegatibleForFree(N1))
> >      return DAG.getNode(ISD::FADD, VT, N0, GetNegatedExpression(N1, DAG));
> >    
> > -  // If either operand is undef, the result is undef
> > -  if (N0.getOpcode() == ISD::UNDEF || N1.getOpcode() == ISD::UNDEF)
> > -    return DAG.getNode(ISD::UNDEF, VT);
> > -
> >    return SDOperand();
> >  }
> 
> Why the difference between fadd and fsub?  If you are worried about NaNs
> etc, at worst you can always, I suppose, turn x fsub undef into x.
> Likewise for the other floating point ops.

Did you mean the difference between integer add and fadd? I just decided
that folding floating-point undef isn't very valuable to me, and the easiest
way to fix the code was to revert the change.

Looking at it a little more, I think the instcombine folds are actually
wrong here. For example, it does X + undef -> undef, but that's wrong if
X is a NaN.

Dan

-- 
Dan Gohman, Cray Inc.