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

[Duncan Sands]

Hi Dan,

> +  // fold (add x, undef) -> undef
> +  if (N1.getOpcode() == ISD::UNDEF)
> +    return N1;

what if N0 is undef and not N1?

> -  // 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?

> -  // 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.

> +  // 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?

> +  // undef / X -> 0
> +  if (N0.getOpcode() == ISD::UNDEF)
> +    return DAG.getConstant(0, VT);

Indeed.

> +  // 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.

> @@ -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.

> +  // 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?

> @@ -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.

Ciao,

Duncan.