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

[Dan Gohman]

Hi Chris,

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

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

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

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

Dan

-- 
Dan Gohman, Cray Inc.