> 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 _______________________________________________ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
