[llvm-commits] CVS: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
clattner at apple.com
Tue Jun 26 09:59:02 PDT 2007
> 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, Ops.size());
>> This idiom occurs in several places. Do you think it makes sense to
>> have a helper method on SelectionDAG to do this?
Thanks for all the great changes Dan!
More information about the llvm-commits