[llvm-commits] DIV -> U/S/FDiv Patch For Review
Chris Lattner
clattner at apple.com
Mon Oct 23 22:52:50 PDT 2006
On Oct 22, 2006, at 7:50 PM, Reid Spencer wrote:
> Attached is a patch to implement splitting the signless DIV
> instruction
> into UDiv (unsigned), SDiv (signed), and FDiv (floating point)
> instructions. This is part of the Signless Types work I'm doing.
Overall, this patch looks like a great first step, but it has
significant flaws. In particular, the patch seems unable to produce
a [su]div instruction with operands of the opposite sign from that of
the instruction. Further, handling of this (very important) case is
seriously buggy in many cases.
I strongly recommend adding a transformation to instcombine that will
produce more divs with such operands. For example, changing
something like:
%X = cast uint %A to int
%Y = sdiv int %X, 1234
%Z = cast int %Y to uint
into:
%Z = sdiv uint %A, 1234
Fortunately, instcombine already does this for other operations (mul/
add/and/or/xor, etc) at line 5658 of Instcombine in CVS. Please add
a case there for UDIV/SDIV. This will hopefully help flush out bugs
in the patch that I didn't catch.
When you get the comments in this email address and things retested,
please resend the patch for review.
Comments preceded with ***'s
--- include/llvm/Instruction.def 8 Apr 2006 01:15:18 -0000 1.19
+++ include/llvm/Instruction.def 23 Oct 2006 02:43:18 -0000
@@ -88,52 +88,50 @@ HANDLE_TERM_INST ( 5, Unwind , Unwi
...
*** Why not reserve some space for all the new instructions you plan
to add? That way the CVS bc format won't be changing as much.
-HANDLE_BINARY_INST( 7, Add , BinaryOperator)
-HANDLE_BINARY_INST( 8, Sub , BinaryOperator)
-HANDLE_BINARY_INST( 9, Mul , BinaryOperator)
+HANDLE_BINARY_INST( 7, Add , BinaryOperator)
+HANDLE_BINARY_INST( 8, Sub , BinaryOperator)
+HANDLE_BINARY_INST( 9, Mul , BinaryOperator)
*** Why remove the space?
--- lib/Analysis/ScalarEvolution.cpp 20 Oct 2006 07:07:24 -0000 1.54
+++ lib/Analysis/ScalarEvolution.cpp 23 Oct 2006 02:43:20 -0000
@@ -1382,12 +1382,12 @@ SCEVHandle ScalarEvolutionsImpl::createS
return SCEVAddExpr::get(getSCEV(I->getOperand(0)),
getSCEV(I->getOperand(1)));
case Instruction::Mul:
return SCEVMulExpr::get(getSCEV(I->getOperand(0)),
getSCEV(I->getOperand(1)));
- case Instruction::Div:
- if (V->getType()->isInteger() && V->getType()->isSigned())
+ case Instruction::SDiv:
+ if (V->getType()->isInteger())
return SCEVSDivExpr::get(getSCEV(I->getOperand(0)),
getSCEV(I->getOperand(1)));
break;
*** No need to check for isInteger anymore.
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/AsmParser/llvmAsmParser.y,v
retrieving revision 1.269
diff -t -d -u -p -5 -r1.269 llvmAsmParser.y
--- lib/AsmParser/llvmAsmParser.y 22 Oct 2006 07:03:09 -0000 1.269
+++ lib/AsmParser/llvmAsmParser.y 23 Oct 2006 02:43:24 -0000
@@ -811,10 +811,41 @@ static PATypeHolder HandleUpRefs(const T
+// This template function is used to obtain the correct opcode for an
+// instruction when an obsolete opcode is encountered. The
OpcodeInfo template
+// keeps track of the opcode and the "obsolete" flag. These are
generated by
+// the lexer and obsolete will be true when the lexer encounters the
token for
+// an obsolete opcode. For example, "div" was replaced by [usf]div
but we need
+// to maintain backwards compatibility for asm files that still have
the "div"
+// instruction. This function handles converting div -> [usf]div
appropriately.
+template <class EnumKind>
+static void sanitizeOpCode(OpcodeInfo<EnumKind> &OI, const
PATypeHolder& Ty) {
+ if (OI.obsolete) {
+ switch (OI.opcode) {
+ default:
+ GenerateError("Invalid Obsolete OpCode");
+ break;
+ case Instruction::UDiv:
+ if (Ty->isFloatingPoint())
+ OI.opcode = Instruction::FDiv;
+ else if (Ty->isSigned())
+ OI.opcode = Instruction::SDiv;
+ break;
+ case Instruction::SDiv:
+ if (Ty->isFloatingPoint())
+ OI.opcode = Instruction::FDiv;
+ else if (Ty->isUnsigned())
+ OI.opcode = Instruction::UDiv;
+ break;
...
*** I like your approach. I don't think sanitizeOpCode should be a
template though. This specific one only works for binops, because
that is what enum values it has. I'd just have one function for each
class of upgraded op, no templates. Also, the lexer can't return an
"obsolete sdiv", it appears to only return udiv, so the sdiv case
above seems dead. Am I missing something? Finally, the code would
be simpler if you started it as "if (!OI.obsolete) return;"
Index: lib/Bytecode/Reader/Reader.cpp
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/Bytecode/Reader/Reader.cpp,v
retrieving revision 1.199
diff -t -d -u -p -5 -r1.199 Reader.cpp
--- lib/Bytecode/Reader/Reader.cpp 20 Oct 2006 07:07:24 -0000 1.199
+++ lib/Bytecode/Reader/Reader.cpp 23 Oct 2006 02:43:26 -0000
@@ -560,10 +560,239 @@ void BytecodeReader::insertArguments(Fun
...
+ // Declare the resulting instruction we might build.
+ Instruction *Result = 0;
+
+ // If this is a bytecode format that did not include the unreachable
+ // instruction, bump up the opcode number to adjust
+ if (hasNoUnreachableInst) {
+ if (Opcode >= Instruction::Unreachable &&
+ Opcode < 62) { // 62
+ ++Opcode;
+ }
+ }
+
+ // First, short circuit this if no conversion is required. When
signless
+ // instructions were implemented the entire opcode sequence was
revised so
+ // we key on this first which means that the opcode value read is
the one
+ // we should use.
+ if (!hasSignlessInstructions)
+ return Result;
*** If hasSignlessInstructions is true, hasNoUnreachableInst is
false, right? If so, move the fast path above the
hasNoUnreachableInst check? It would be more clear to return 0 here
explicitly and mention in the method comment that this returns null
if no upgrading is needed.
+// Upgrade obsolte constant expression opcodes (ver. 5 and prior) to
the new
*** Typo obsolete.
+ // need to note that we have signed integer types in prior
versions.
+ hasSignedIntegers = true;
+
+ // FALL THROUGH
+
+ case 6: // SignlessTypes Implementation (1.10 release)
*** 1.9?
Index: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/CodeGen/SelectionDAG/
SelectionDAGISel.cpp,v
retrieving revision 1.294
diff -t -d -u -p -5 -r1.294 SelectionDAGISel.cpp
--- lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp 22 Oct 2006
23:00:53 -0000 1.294
+++ lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp 23 Oct 2006
02:43:28 -0000
+void
+SelectionDAGLowering::visitIntBinary(User &I, unsigned IntOp,
unsigned VecOp) {
const Type *Ty = I.getType();
SDOperand Op1 = getValue(I.getOperand(0));
SDOperand Op2 = getValue(I.getOperand(1));
if (Ty->isIntegral()) {
setValue(&I, DAG.getNode(IntOp, Op1.getValueType(), Op1, Op2));
- } else if (Ty->isFloatingPoint()) {
+ } else {
+ const PackedType *PTy = cast<PackedType>(Ty);
+ SDOperand Num = DAG.getConstant(PTy->getNumElements(), MVT::i32);
+ SDOperand Typ = DAG.getValueType(TLI.getValueType(PTy-
>getElementType()));
+ setValue(&I, DAG.getNode(VecOp, MVT::Vector, Op1, Op2, Num, Typ));
+ }
+}
*** In visitIntBinary/visitFPBinary please change the condition to
"if (const PackedType *PTy = dyn_cast<PackedType>(Ty)) {", swapping
the order of the if conditions. Checking "isIntegral" is a non-
obvious way to check that it's not a vector type (your code is
correct, just not obvious what it does).
--- lib/ExecutionEngine/Interpreter/Execution.cpp 20 Oct 2006
07:07:24 -0000 1.140
+++ lib/ExecutionEngine/Interpreter/Execution.cpp 23 Oct 2006
02:43:29 -0000
@@ -87,11 +87,13 @@ GenericValue Interpreter::getConstantExp
CE->getOperand(0)->getType());
case Instruction::Mul:
return executeMulInst(getOperandValue(CE->getOperand(0), SF),
getOperandValue(CE->getOperand(1), SF),
CE->getOperand(0)->getType());
- case Instruction::Div:
+ case Instruction::SDiv:
+ case Instruction::UDiv:
+ case Instruction::FDiv:
return executeDivInst(getOperandValue(CE->getOperand(0), SF),
getOperandValue(CE->getOperand(1), SF),
CE->getOperand(0)->getType());
case Instruction::Rem:
return executeRemInst(getOperandValue(CE->getOperand(0), SF),
@@ -502,11 +504,13 @@ void Interpreter::visitBinaryOperator(Bi
switch (I.getOpcode()) {
case Instruction::Add: R = executeAddInst (Src1, Src2, Ty);
break;
case Instruction::Sub: R = executeSubInst (Src1, Src2, Ty);
break;
case Instruction::Mul: R = executeMulInst (Src1, Src2, Ty);
break;
- case Instruction::Div: R = executeDivInst (Src1, Src2, Ty); break;
+ case Instruction::SDiv:
+ case Instruction::UDiv:
+ case Instruction::FDiv: R = executeDivInst (Src1, Src2, Ty); break;
case Instruction::Rem: R = executeRemInst (Src1, Src2, Ty);
break;
case Instruction::And: R = executeAndInst (Src1, Src2, Ty);
break;
case Instruction::Or: R = executeOrInst (Src1, Src2, Ty);
break;
case Instruction::Xor: R = executeXorInst (Src1, Src2, Ty);
break;
case Instruction::SetEQ: R = executeSetEQInst(Src1, Src2, Ty);
break;
*** This is flat out incorrect. It should have separate
execute*DivInst functions that do the operation not based on the
type. This would misinterpret 'udiv int -4, 2' for example. This
must be fixed before you checkin.
--- lib/Target/CBackend/Writer.cpp 22 Oct 2006 09:58:21 -0000 1.274
+++ lib/Target/CBackend/Writer.cpp 23 Oct 2006 02:43:30 -0000
@@ -584,11 +584,13 @@ void CWriter::printConstant(Constant *CP
@@ -603,11 +605,13 @@ void CWriter::printConstant(Constant *CP
printConstant(CE->getOperand(0));
switch (CE->getOpcode()) {
case Instruction::Add: Out << " + "; break;
case Instruction::Sub: Out << " - "; break;
case Instruction::Mul: Out << " * "; break;
- case Instruction::Div: Out << " / "; break;
+ case Instruction::UDiv:
+ case Instruction::SDiv:
+ case Instruction::FDiv: Out << " / "; break;
case Instruction::Rem: Out << " % "; break;
case Instruction::And: Out << " & "; break;
case Instruction::Or: Out << " | "; break;
case Instruction::Xor: Out << " ^ "; break;
case Instruction::SetEQ: Out << " == "; break;
*** The CBE has the same problem. You need to force the operands to
the right sign in the C output so that the C compiler will generate
the appropriately signed operation. This must be fixed before you
checkin.
--- lib/Transforms/IPO/SimplifyLibCalls.cpp 20 Oct 2006 07:07:24
-0000 1.70
+++ lib/Transforms/IPO/SimplifyLibCalls.cpp 23 Oct 2006 02:43:30 -0000
@@ -1273,11 +1273,11 @@ public:
ci->replaceAllUsesWith(base);
ci->eraseFromParent();
return true;
} else if (Op2V == -1.0) {
// pow(x,-1.0) -> 1.0/x
- BinaryOperator* div_inst= BinaryOperator::createDiv(
+ BinaryOperator* div_inst= BinaryOperator::createSDiv(
ConstantFP::get(Ty,1.0), base, ci->getName()+".pow", ci);
ci->replaceAllUsesWith(div_inst);
ci->eraseFromParent();
return true;
}
*** Shouldn't this be fdiv? Please fix before you checkin, and add a
regression test.
Index: lib/Transforms/Scalar/InstructionCombining.cpp
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/Transforms/Scalar/
InstructionCombining.cpp,v
retrieving revision 1.527
diff -t -d -u -p -5 -r1.527 InstructionCombining.cpp
--- lib/Transforms/Scalar/InstructionCombining.cpp 20 Oct 2006
18:20:21 -0000 1.527
+++ lib/Transforms/Scalar/InstructionCombining.cpp 23 Oct 2006
02:43:34 -0000
@@ -1973,15 +1979,15 @@ Instruction *InstCombiner::visitSub(Bina
InsertNewInstBefore(BinaryOperator::createNot(OtherOp,
"B.not"), I);
return BinaryOperator::createAnd(Op0, NewNot);
}
// 0 - (X sdiv C) -> (X sdiv -C)
- if (Op1I->getOpcode() == Instruction::Div)
+ if (Op1I->getOpcode() == Instruction::SDiv)
if (ConstantInt *CSI = dyn_cast<ConstantInt>(Op0))
if (CSI->getType()->isSigned() && CSI->isNullValue())
if (Constant *DivRHS = dyn_cast<Constant>(Op1I-
>getOperand(1)))
- return BinaryOperator::createDiv(Op1I->getOperand(0),
+ return BinaryOperator::createSDiv(Op1I->getOperand(0),
ConstantExpr::getNeg
(DivRHS));
*** You should be able to drop the 'CSI->getType()->isSigned()' check.
+ // (X / C1) / C2 -> X / (C1*C2)
if (Instruction *LHS = dyn_cast<Instruction>(Op0))
- if (LHS->getOpcode() == Instruction::Div)
+ if (LHS->getOpcode() == Instruction::SDiv ||
+ LHS->getOpcode()==Instruction::UDiv ||
+ LHS->getOpcode()==Instruction::FDiv)
*** This isn't quite right. This will miscompile ((X sdiv C1) udiv
C2). You want something like:
// (X / C1) / C2 -> X / (C1*C2)
if (Instruction *LHS = dyn_cast<Instruction>(Op0))
if (LHS->getOpcode() == I.getOpcode())
which is also simpler.
if (ConstantInt *LHSRHS = dyn_cast<ConstantInt>(LHS-
>getOperand(1))) {
- // (X / C1) / C2 -> X / (C1*C2)
- return BinaryOperator::createDiv(LHS->getOperand(0),
- ConstantExpr::getMul(RHS,
LHSRHS));
+ return BinaryOperator::create(
+ Instruction::BinaryOps(LHS->getOpcode()), LHS->getOperand
(0),
+ ConstantExpr::getMul(RHS,
LHSRHS));
}
*** If you use I.getOpcode(), you can drop the BinaryOps cast.
*** Why not have commonIDivTransforms call commonDivTransforms? It
would be nicer to just have:
+ if (Instruction *Common = commonIDivTransforms(I))
+ return Common;
Also, please try to stick with the established style when modifying
existing code. In this case, putting the '*' in the right place,
capitalizing variables, etc.
+ // Check to see if this is an unsigned division with an exact
power of 2,
+ // if so, convert to a right shift.
+ // X udiv C^2 -> X >> C
+ if (ConstantInt *C = dyn_cast<ConstantInt>(Op1)) {
+ if (uint64_t Val = C->getZExtValue()) // Don't break X / 0
+ if (isPowerOf2_64(Val)) {
+ uint64_t C = Log2_64(Val);
+ return new ShiftInst(Instruction::Shr, Op0,
+ ConstantInt::get(Type::UByteTy, C));
+ }
+ }
*** This will assert and die on something like 'udiv int %C, 64'.
You need to insert casts of the input value and of the output result
if the operands/result is signed. This specific issue goes away when
shifts are split up.
+ if (Instruction *RHSI = dyn_cast<Instruction>(I.getOperand(1))) {
+ // Turn A / (C1 << N), where C1 is "1<<C2" into A >> (N+C2)
[udiv only].
+ if (RHSI->getOpcode() == Instruction::Shl &&
+ isa<ConstantInt>(RHSI->getOperand(0)) &&
+ RHSI->getOperand(0)->getType()->isUnsigned()) {
+ uint64_t C1 = cast<ConstantInt>(RHSI->getOperand(0))-
>getZExtValue();
+ if (isPowerOf2_64(C1)) {
+ uint64_t C2 = Log2_64(C1);
+ Value *Add = RHSI->getOperand(1);
+ if (C2) {
+ Constant *C2V = ConstantInt::get(Add->getType(), C2);
+ Add = InsertNewInstBefore(BinaryOperator::createAdd(Add, C2V,
+
"tmp"), I);
}
+ return new ShiftInst(Instruction::Shr, Op0, Add);
}
}
}
*** This code doesn't need to check 'RHSI->getOperand(0)->getType()-
>isUnsigned()', but that will make you have to handle the signed
case right (inserting casts).
+ // If the sign bits of both operands are zero (i.e. we can prove
they are
+ // unsigned inputs), turn this into a udiv.
+ uint64_t Mask = 1ULL << (I.getType()->getPrimitiveSizeInBits()-1);
+ if (MaskedValueIsZero(Op1, Mask) && MaskedValueIsZero(Op0, Mask)) {
+ const Type *NTy = Op0->getType()->getUnsignedVersion();
+ Instruction *LHS = new CastInst(Op0, NTy, Op0->getName());
+ InsertNewInstBefore(LHS, I);
+ Value *RHS;
+ if (Constant *R = dyn_cast<Constant>(Op1))
+ RHS = ConstantExpr::getCast(R, NTy);
+ else
+ RHS = InsertNewInstBefore(new CastInst(Op1, NTy, Op1->getName
()), I);
+ Instruction *Div = BinaryOperator::createUDiv(LHS, RHS, I.getName
());
+ InsertNewInstBefore(Div, I);
+ return new CastInst(Div, I.getType());
+ }
*** This code gets much simpler now. Try:
uint64_t Mask = 1ULL << (I.getType()->getPrimitiveSizeInBits()-1);
if (MaskedValueIsZero(Op1, Mask) && MaskedValueIsZero(Op0, Mask))
return BinaryOperator::createUDiv(LHS, RHS, I.getName());
The cast sequence *is* needed for converting stuff to shifts etc, above.
+ // Handle div X, Cond?Y:Z
+ if (SelectInst *SI = dyn_cast<SelectInst>(Op1)) {
+ // div X, (Cond ? 0 : Y) -> div X, Y. If the div and the select
are in the
+ // same basic block, then we replace the select with Y, and the
condition of
+ // the select with false (if the cond value is in the same BB).
If the
+ // select has uses other than the div, this allows them to be
simplified
+ // also.
+ if (Constant *ST = dyn_cast<Constant>(SI->getOperand(1)))
+ if (ST->isNullValue()) {
+ Instruction *CondI = dyn_cast<Instruction>(SI->getOperand(0));
+ if (CondI && CondI->getParent() == I.getParent())
+ UpdateValueUsesWith(CondI, ConstantBool::getFalse());
+ else if (I.getParent() != SI->getParent() || SI->hasOneUse())
+ I.setOperand(1, SI->getOperand(2));
+ else
+ UpdateValueUsesWith(SI, SI->getOperand(2));
+ return &I;
+ }
+
+ // Likewise for: div X, (Cond ? Y : 0) -> div X, Y
+ if (Constant *ST = dyn_cast<Constant>(SI->getOperand(2)))
+ if (ST->isNullValue()) {
+ Instruction *CondI = dyn_cast<Instruction>(SI->getOperand(0));
+ if (CondI && CondI->getParent() == I.getParent())
+ UpdateValueUsesWith(CondI, ConstantBool::getTrue());
+ else if (I.getParent() != SI->getParent() || SI->hasOneUse())
+ I.setOperand(1, SI->getOperand(1));
+ else
+ UpdateValueUsesWith(SI, SI->getOperand(1));
+ return &I;
+ }
+ }
*** This sequence should be in commonDivTransforms, no? You have
cloned this code in commonIDivTransforms, please merge the two copies.
Another significant issue not obvious from your diff is that you left
this hunk of code:
// If this is 'udiv X, (Cond ? C1, C2)' where C1&C2 are powers
of two,
// transform this into: '(Cond ? (udiv X, C1) : (udiv X, C2))'.
if (ConstantInt *STO = dyn_cast<ConstantInt>(SI->getOperand(1)))
if (ConstantInt *SFO = dyn_cast<ConstantInt>(SI->getOperand(2)))
if (STO->getType()->isUnsigned() && SFO->getType()-
>isUnsigned()) {
// STO == 0 and SFO == 0 handled above.
uint64_t TVA = STO->getZExtValue(), FVA = SFO->getZExtValue
();
if (isPowerOf2_64(TVA) && isPowerOf2_64(FVA)) {
unsigned TSA = Log2_64(TVA), FSA = Log2_64(FVA);
Constant *TC = ConstantInt::get(Type::UByteTy, TSA);
Instruction *TSI = new ShiftInst(Instruction::Shr, Op0,
TC, SI->getName()+".t");
TSI = InsertNewInstBefore(TSI, I);
Constant *FC = ConstantInt::get(Type::UByteTy, FSA);
Instruction *FSI = new ShiftInst(Instruction::Shr, Op0,
FC, SI->getName()+".f");
FSI = InsertNewInstBefore(FSI, I);
return new SelectInst(SI->getOperand(0), TSI, FSI);
}
}
in the commonIDivTransforms method, but it is specific to udiv.
Please move it to the udiv case, and make it insert casts etc as needed.
@@ -3720,11 +3803,13 @@ Instruction *InstCombiner::visitXor(Bina
/// MulWithOverflow - Compute Result = In1*In2, returning true if
the result
/// overflowed for this type.
static bool MulWithOverflow(ConstantInt *&Result, ConstantInt *In1,
ConstantInt *In2) {
Result = cast<ConstantInt>(ConstantExpr::getMul(In1, In2));
- return !In2->isNullValue() && ConstantExpr::getDiv(Result, In2) !=
In1;
+ return !In2->isNullValue() && (In2->getType()->isSigned() ?
+ ConstantExpr::getSDiv(Result, In2) :
+ ConstantExpr::getUDiv(Result, In2)) != In1;
}
static bool isPositive(ConstantInt *C) {
return C->getSExtValue() >= 0;
}
*** This is subtly buggy. If you look at how MulWithOverflow is
used, it is called from the "Fold: (div X, C1) op C2 -> range check"
case. You need to pass in whether or not the *operation* is signed
or unsigned, ignoring the sign of the values.
@@ -4377,11 +4462,12 @@ Instruction *InstCombiner::visitSetCondI
}
}
}
break;
- case Instruction::Div:
+ case Instruction::SDiv:
+ case Instruction::UDiv:
// Fold: (div X, C1) op C2 -> range check
if (ConstantInt *DivRHS = dyn_cast<ConstantInt>(LHSI-
>getOperand(1))) {
// Fold this div into the comparison, producing a range
check.
// Determine, based on the divide type, what the range is
being
// checked. If there is an overflow on the low or high
side, remember
*** Likewise, this is extremely buggy. The original code uses
knowledge of the signedness of the operands to determine the
signedness of the comparisons it makes. For example, the code (which
your diff doesn't include) has stuff like:
} else if (LHSI->getType()->isUnsigned()) { // udiv
LoBound = Prod;
LoOverflow = ProdOV;
HiOverflow = ProdOV || AddWithOverflow(HiBound, LoBound,
DivRHS);
... this is looking at the type of the operands, not the operation
code, this *must* be fixed. Later it has:
else if (HiOverflow)
return new SetCondInst(Instruction::SetGE, X, LoBound);
else if (LoOverflow)
return new SetCondInst(Instruction::SetLT, X, HiBound);
These create signed/unsigned comparisons where the sign matched the
sign of the operator, because they follow the sign of the operands.
This needs to be fixed to cast the operands to be the same sign as
the sign of the opcode.
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/VMCore/ConstantFolding.cpp,v
retrieving revision 1.94
diff -t -d -u -p -5 -r1.94 ConstantFolding.cpp
--- lib/VMCore/ConstantFolding.cpp 20 Oct 2006 07:07:24 -0000 1.94
+++ lib/VMCore/ConstantFolding.cpp 23 Oct 2006 02:43:35 -0000
@@ -491,20 +507,32 @@ struct VISIBILITY_HIDDEN DirectIntRules
DEF_CAST(ULong , ConstantInt, uint64_t)
DEF_CAST(Float , ConstantFP , float)
DEF_CAST(Double, ConstantFP , double)
#undef DEF_CAST
- static Constant *Div(const ConstantInt *V1, const ConstantInt *V2) {
- if (V2->isNullValue()) return 0;
+ static Constant *UDiv(const ConstantInt *V1, const ConstantInt *V2) {
+ if (V2->isNullValue())
+ return 0;
if (V2->isAllOnesValue() && // MIN_INT / -1
(BuiltinType)V1->getZExtValue() == -(BuiltinType)V1-
>getZExtValue())
return 0;
BuiltinType R =
(BuiltinType)V1->getZExtValue() / (BuiltinType)V2-
>getZExtValue();
return ConstantInt::get(*Ty, R);
}
+ static Constant *SDiv(const ConstantInt *V1, const ConstantInt *V2) {
+ if (V2->isNullValue())
+ return 0;
+ if (V2->isAllOnesValue() && // MIN_INT / -1
+ (BuiltinType)V1->getSExtValue() == -(BuiltinType)V1-
>getSExtValue())
+ return 0;
+ BuiltinType R =
+ (BuiltinType)V1->getSExtValue() / (BuiltinType)V2->getSExtValue
();
+ return ConstantInt::get(*Ty, R);
+ }
+
You properly sign/zero extend the values here, but then proceed to do
the wrong division. For example, this will do signed division for
'udiv int -2, 2'. What does the -constfold pass produce for:
int %test() { %X = udiv int -2, 2 ret int %X }
I bet it is folded to -1, which is incorrect.
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/VMCore/Constants.cpp,v
retrieving revision 1.165
diff -t -d -u -p -5 -r1.165 Constants.cpp
--- lib/VMCore/Constants.cpp 20 Oct 2006 07:07:24 -0000 1.165
+++ lib/VMCore/Constants.cpp 23 Oct 2006 02:43:36 -0000
..
@@ -444,12 +446,18 @@ Constant *ConstantExpr::getSub(Constant
return get(Instruction::Sub, C1, C2);
}
Constant *ConstantExpr::getMul(Constant *C1, Constant *C2) {
return get(Instruction::Mul, C1, C2);
}
-Constant *ConstantExpr::getDiv(Constant *C1, Constant *C2) {
- return get(Instruction::Div, C1, C2);
+Constant *ConstantExpr::getUDiv(Constant *C1, Constant *C2) {
+ return get(Instruction::UDiv, C1, C2);
+}
+Constant *ConstantExpr::getSDiv(Constant *C1, Constant *C2) {
+ return get(Instruction::SDiv, C1, C2);
+}
+Constant *ConstantExpr::getFDiv(Constant *C1, Constant *C2) {
+ return get(Instruction::FDiv, C1, C2);
}
Constant *ConstantExpr::getRem(Constant *C1, Constant *C2) {
return get(Instruction::Rem, C1, C2);
}
Constant *ConstantExpr::getAnd(Constant *C1, Constant *C2) {
*** This would be an excellent place to assert that the arguments are
integer or fp (or vectors of) as appropriate.
===================================================================
RCS file: /var/cvs/llvm/llvm/lib/VMCore/Instructions.cpp,v
retrieving revision 1.43
diff -t -d -u -p -5 -r1.43 Instructions.cpp
--- lib/VMCore/Instructions.cpp 20 Oct 2006 07:07:24 -0000 1.43
+++ lib/VMCore/Instructions.cpp 23 Oct 2006 02:43:36 -0000
@@ -1020,11 +1020,11 @@ void BinaryOperator::init(BinaryOps iTyp
assert(LHS->getType() == RHS->getType() &&
"Binary operator operand types must match!");
#ifndef NDEBUG
switch (iType) {
case Add: case Sub:
- case Mul: case Div:
+ case Mul: case UDiv: case SDiv: case FDiv:
case Rem:
assert(getType() == LHS->getType() &&
"Arithmetic operation should return same type as
operands!");
assert((getType()->isInteger() || getType()->isFloatingPoint() ||
isa<PackedType>(getType())) &&
*** Likewise, this would be a good place to assert that the operands
are int/fp(or vectors of) as appropriate.
-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20061023/5f225fa0/attachment.html>
More information about the llvm-commits
mailing list