On Oct 25, 2006, at 12:15 PM, Reid Spencer wrote:
Overall, this patch looks very nice. Please fix the one bug below (adding the regtest) and you are approved to check the whole mess in. Final part (instcombine): +Instruction *InstCombiner::visitFDiv(BinaryOperator &I) { + Value *Op0 = I.getOperand(0), *Op1 = I.getOperand(1); + + return commonDivTransforms(I); +} You can delete Op0/Op1. + // FIXME: If the operand types don't match the type of the divide + // then don't attempt this transform. The code below doesn't have the + // logic to deal with a signed divide and an unsigned compare (and + // vice versa). This is because (x /s C1) <s C2 produces different + // results than (x /s C1) <u C2 or (x /u C1) <s C2 or even + // (x /u C1) <u C2. Simply casting the operands and result won't + // work. :( The if statement below tests that condition and bails + // if it finds it. + const Type* DivRHSTy = DivRHS->getType(); + unsigned DivOpCode = LHSI->getOpcode(); + if ((DivOpCode == Instruction::SDiv && DivRHSTy->isUnsigned()) || + (DivOpCode == Instruction::UDiv && DivRHSTy->isSigned())) + break; This is good but slightly over conservative. Please allow the xform to happen if the comparison is an ==/!= comparison (i.e. SetCondInst::isEquality() returns true). Instruction* InstCombiner::commonDivTransforms(BinaryOperator &I) { Please add comments above each new function. The visit* methods don't need comments above them. // (X / C1) / C2 -> X / (C1*C2) if (Instruction *LHS = dyn_cast<Instruction>(Op0)) if (Instruction::BinaryOps(LHS->getOpcode()) == I.getOpcode()) This code is a bit simpler as: // (X / C1) / C2 -> X / (C1*C2) if (BinaryOperator *LHS = dyn_cast<BinaryOperator>(Op0)) if (LHS->getOpcode() == I.getOpcode()) // udiv X, (Select Cond, C1, C2) --> Select Cond, (shr X, C1), (shr X, C2) // where C1&C2 are powers of two. ... X = InsertNewInstBefore( new CastInst(X, X->getType()->getUnsignedVersion()), I); This (and similar cases) is easier/cleaner with InsertCastBefore. // Finally, construct the select instruction and return it. return new SelectInst(SI->getOperand(0), TSI, FSI); In the same xform, this needs to cast the result back to the right type if the operation was signed. Please write a short regtest for this and check it in with your patch to verify that you get this right (add it to the end of div.ll or something). Something like this: int %test(int %X, int %Y, bool %C) { %A = select bool %C, int 1024, int 32 %B = udiv int %Y, %A ret int %B } -Chris |
_______________________________________________ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
