[llvm-commits] CVS: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

[Chris Lattner]

> Thanks for all the review comments! I've addressed a few comments
> below; I'll get to the others soon.

Thanks!  I also filed pr1529, which is the only failure that showed  
on the ppc nightly tester.

>>> @@ -1162,6 +1179,11 @@
>>>      SDOperand Op = BuildSDIV(N);
>>>      if (Op.Val) return Op;
>>>    }
>>> +
>>> +  // 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();
>>>  }
>>
>> This is not safe for sdiv/udiv.  Safe xforms are:
>>
>>    // undef / X -> 0
>>    // X / undef -> undef
>>
>> If in doubt, plz check instcombine.
>
> Thanks for correcting me on the undef rules. I'll check in a fix
> for the code soon. For this sdiv/udiv one though, why is undef/X not
> undef? For any non-zero value of X there's at least one value the
> undef might have which makes the divide have a non-zero result.

I think that undef udiv intmax -> 0, no?  If not, plz update  
instcombine as well.


>>> @@ -2742,6 +2807,30 @@
>>>    SDOperand N0 = N->getOperand(0);
>>>    MVT::ValueType VT = N->getValueType(0);
>>>
>>> +  // If the input is a BUILD_VECTOR with all constant elements,
>>> fold this now.
>>> +  // Only do this before legalize, since afterward the target may
>>> be depending
>>> +  // on the bitconvert.
>>
>> Interesting.  This is a good solution for now, but maybe this argues
>> for having a "target build vector", like "target constant", which
>> would be unmolested by the optimizer?
>
> It's a similar situation for SimplifyVBinOp, and a few other  
> places, as
> you noticed, and we don't want to clone all those if we don't need to.
> An alternative would be to replace the conservative checks with
> specific checks for
>    !AfterLegalize || TLI.isOperationLegal(....)
> as is done in other places to protect against creating illegal nodes.

I think it certainly would be better to check isOperationLegal.  The  
only hard part is that some operations (like buildvector) are legal  
with certain operands.  We don't have a way to capture that. :(

>>> +    MVT::ValueType VT = MVT::getVectorType(DstEltVT,
>>> +                                           Ops.size());
>>> +    return DAG.getNode(ISD::BUILD_VECTOR, VT, &Ops[0], Ops.size());
>>
>> This idiom occurs in several places.  Do you think it makes sense to
>> have a helper method on SelectionDAG to do this?
>
> Sure.

Thanks for all the great changes Dan!

-Chris