[llvm-commits] SETCC Patches: #3
Reid Spencer
rspencer at reidspencer.com
Thu Dec 14 19:48:09 PST 2006
Hi Chris,
Some feedback on your review ..
On Mon, 2006-12-11 at 22:51 -0800, Chris Lattner wrote:
>
> On Dec 10, 2006, at 3:43 PM, Reid Spencer wrote:
>
> > All,
> >
> >
> > Here's the SETCC patch to convert SetCondInst (SetCC) instructions
> > into
> > ICmpInst. Here's somethings you should know about this patch:
>
>
>
> This is all the rest, except for instcombine.
Most of the changes you suggested were just done. There's a couple
questions below that I need the answers for.
>
>
> Index: tools/llvm-upgrade/UpgradeParser.y
> ===================================================================
> RCS file: /var/cvs/llvm/llvm/tools/llvm-upgrade/UpgradeParser.y,v
> retrieving revision 1.24
> diff -t -d -u -p -5 -r1.24 UpgradeParser.y
> --- tools/llvm-upgrade/UpgradeParser.y 9 Dec 2006 19:40:41 -0000 1.24
> +++ tools/llvm-upgrade/UpgradeParser.y 10 Dec 2006 23:27:00 -0000
> @@ -21,11 +21,11 @@
> #include <cassert>
>
>
>
> #define YYERROR_VERBOSE 1
> #define YYINCLUDED_STDLIB_H
> #define YYDEBUG 1
> -#define UPGRADE_SETCOND_OPS 0
> +#define UPGRADE_SETCOND_OPS 1
> #define GENERATE_FCMP_INSTS 0
>
>
>
> int yylex(); // declaration" of xxx warnings.
> int yyparse();
> extern int yydebug;
>
>
> This #ifdef should go away now, since it's always on.
Yup. Done.
>
>
>
>
> Index: lib/ExecutionEngine/Interpreter/Execution.cpp
> ===================================================================
> RCS
> file: /var/cvs/llvm/llvm/lib/ExecutionEngine/Interpreter/Execution.cpp,v
> retrieving revision 1.152
> diff -t -d -u -p -5 -r1.152 Execution.cpp
> --- lib/ExecutionEngine/Interpreter/Execution.cpp 7 Dec 2006 01:30:31
> -0000 1.152
> +++ lib/ExecutionEngine/Interpreter/Execution.cpp 10 Dec 2006 23:26:54
> -0000
> ...
>
>
> +static GenericValue executeICMP_ULT(GenericValue Src1, GenericValue
> Src2,
> + const Type *Ty) {
> + GenericValue Dest;
> + switch (Ty->getTypeID()) {
> + IMPLEMENT_CMP(<, UByte);
> + IMPLEMENT_CMP(<, UShort);
> + IMPLEMENT_CMP(<, UInt);
> + IMPLEMENT_CMP(<, ULong);
> + IMPLEMENT_POINTERCMP(<);
> + default:
> + cerr << "Unhandled type for ICMP_ULT predicate: " << *Ty << "\n";
> + abort();
> + }
> + return Dest;
> +}
>
>
> The interpreter is broken: all unsigned comparisons with signed
> operands will hit the unhandled type case.
Yup. Fixed the macro to take two arguments. The first is to match the
type of the operand.
The second is the type to use to pull the values out of the GenericValue
union, effecitvely doing the type cast by field selection.
>
>
>
>
> Index: lib/Target/CBackend/Writer.cpp
> ===================================================================
> RCS file: /var/cvs/llvm/llvm/lib/Target/CBackend/Writer.cpp,v
> retrieving revision 1.294
> diff -t -d -u -p -5 -r1.294 Writer.cpp
> --- lib/Target/CBackend/Writer.cpp 7 Dec 2006 23:41:45 -0000 1.294
> +++ lib/Target/CBackend/Writer.cpp 10 Dec 2006 23:26:54 -0000
> @@ -712,10 +717,44 @@ void CWriter::printConstant(Constant *CP
> case Instruction::SetGT: Out << " > "; break;
> case Instruction::SetGE: Out << " >= "; break;
> case Instruction::Shl: Out << " << "; break;
> case Instruction::LShr:
> case Instruction::AShr: Out << " >> "; break;
> + case Instruction::ICmp:
> + switch (CE->getPredicate()) {
> + case ICmpInst::ICMP_EQ: Out << " == "; break;
> + case ICmpInst::ICMP_NE: Out << " != "; break;
> + case ICmpInst::ICMP_SLT:
> + case ICmpInst::ICMP_ULT: Out << " < "; break;
> + case ICmpInst::ICMP_SLE:
> + case ICmpInst::ICMP_ULE: Out << " <= "; break;
> ...
>
>
> This apparently doesn't cast the operands of icmp constant exprs to
> the right sign.
It does, you just didn't see it in the patch because that part didn't
change.
>
>
>
>
>
>
> @@ -1122,10 +1161,65 @@ void CWriter::writeOperandWithCast(Value
> +
> +// generateCompilerSpecificCode - This is where we add conditional
> compilation
> +// directives to cater to specific compilers as need be.
> +//
> +
> // generateCompilerSpecificCode - This is where we add conditional
> compilation
> // directives to cater to specific compilers as need be.
> //
>
>
> ... There ya go again, trying to inflate your LOC count.
Just a blunder.
>
>
>
>
> @@ -1982,10 +2076,49 @@ void CWriter::visitBinaryOperator(Instru
> if (needsCast) {
> Out << "))";
> }
> }
>
>
>
> +void CWriter::visitICmpInst(Instruction &I) {
> + // icmp instruction.
> + assert(isa<ICmpInst>(I));
> +
>
>
> Drop the isa and later casts, change the argument to visitICmpInst to
> ICmpInst.
Yup. The patch didn't catch up with the visitICmpInst change. This was
actually a bug. It provided an overload of visitICmpInst that would
never be called. Unfortunately there's no -Woverload-nonvirtual
option :)
>
>
>
>
> Index: lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
> ===================================================================
> RCS
> file: /var/cvs/llvm/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp,v
> retrieving revision 1.326
> diff -t -d -u -p -5 -r1.326 SelectionDAGISel.cpp
> --- lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp 9 Dec 2006 02:42:38
> -0000 1.326
> +++ lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp 10 Dec 2006 23:26:53
> -0000
>
> + BOp = cast<Instruction>(Cond);
> + ISD::CondCode Condition;
> + if (SetCondInst *SCI = dyn_cast<SetCondInst>(Cond)) {
> + ISD::CondCode SignCond, UnsCond, FPCond;
> + switch (BOp->getOpcode()) {
> + default: assert(0 && "Unknown setcc opcode!");
> + case Instruction::SetEQ:
> + SignCond = ISD::SETEQ;
> + UnsCond = ISD::SETEQ;
> + FPCond = ISD::SETOEQ;
> + break;
> + case Instruction::SetNE:
> + SignCond = ISD::SETNE;
> + UnsCond = ISD::SETNE;
> + FPCond = ISD::SETUNE;
> + break;
> + case Instruction::SetLE:
> + SignCond = ISD::SETLE;
> + UnsCond = ISD::SETULE;
> + FPCond = ISD::SETOLE;
> + break;
> + case Instruction::SetGE:
> + SignCond = ISD::SETGE;
> + UnsCond = ISD::SETUGE;
> + FPCond = ISD::SETOGE;
> + break;
> + case Instruction::SetLT:
> + SignCond = ISD::SETLT;
> + UnsCond = ISD::SETULT;
> + FPCond = ISD::SETOLT;
> + break;
> + case Instruction::SetGT:
> + SignCond = ISD::SETGT;
> + UnsCond = ISD::SETUGT;
> + FPCond = ISD::SETOGT;
> + break;
> + }
> + const Type *OpType = SCI->getOperand(0)->getType();
> + if (const PackedType *PTy = dyn_cast<PackedType>(OpType))
> + OpType = PTy->getElementType();
> +
> + if (!FiniteOnlyFPMath() && OpType->isFloatingPoint())
> + Condition = FPCond;
> + else if (OpType->isUnsigned())
> + Condition = UnsCond;
> + else
> + Condition = SignCond;
>
>
> You can drop UnsCond here, as SetCondInst doesn't apply to integers
> anymore.
Yup. Done. I changed it to FOCond (FiniteOnly) and FPCond.
>
>
>
>
> + } else if (FCmpInst *FC = dyn_cast<FCmpInst>(Cond)) {
> + switch (FC->getPredicate()) {
> + default: assert(0 && "Unknown fcmp predicate opcode!");
> + case FCmpInst::FCMP_FALSE: Condition = ISD::SETFALSE; break;
> + case FCmpInst::FCMP_OEQ: Condition = ISD::SETOEQ; break;
> + case FCmpInst::FCMP_OGT: Condition = ISD::SETOGT; break;
> + case FCmpInst::FCMP_OGE: Condition = ISD::SETOGE; break;
> + case FCmpInst::FCMP_OLT: Condition = ISD::SETOLT; break;
> + case FCmpInst::FCMP_OLE: Condition = ISD::SETOLE; break;
> + case FCmpInst::FCMP_ONE: Condition = ISD::SETONE; break;
> + case FCmpInst::FCMP_ORD: Condition = ISD::SETO; break;
> + case FCmpInst::FCMP_UNO: Condition = ISD::SETUO; break;
> + case FCmpInst::FCMP_UEQ: Condition = ISD::SETUEQ; break;
> + case FCmpInst::FCMP_UGT: Condition = ISD::SETUGT; break;
> + case FCmpInst::FCMP_UGE: Condition = ISD::SETUGE; break;
> + case FCmpInst::FCMP_ULT: Condition = ISD::SETULT; break;
> + case FCmpInst::FCMP_ULE: Condition = ISD::SETULE; break;
> + case FCmpInst::FCMP_UNE: Condition = ISD::SETUNE; break;
> + case FCmpInst::FCMP_TRUE: Condition = ISD::SETTRUE; break;
> + }
>
>
> This (when it becomes active) isn't sufficient. When FiniteOnlyFPMath
> is enabled, all the 'o's and 'u's should get dropped.
Right, missed that. One question:
For the FCMP_ORD and FCMP_UNO cases, what should the FininteOnlyFPMath
condition be?
There isn't really a mapping for it, right? Just make it the same as the
regular case?
>
>
> void SelectionDAGLowering::visitFCmp(User &I) {
> + FCmpInst::Predicate predicate = FCmpInst::BAD_FCMP_PREDICATE;
> + if (FCmpInst *FC = dyn_cast<FCmpInst>(&I))
>
>
> likewise.
Right.
>
>
> diff -t -d -u -p -5 -r1.107 SimplifyCFG.cpp
> --- lib/Transforms/Utils/SimplifyCFG.cpp 27 Nov 2006 01:05:10 -0000
> 1.107
> +++ lib/Transforms/Utils/SimplifyCFG.cpp 10 Dec 2006 23:26:51 -0000
> @@ -388,23 +390,43 @@ static bool DominatesMergePoint(Value *V
> }
>
>
>
> return true;
> }
>
>
>
> -// GatherConstantSetEQs - Given a potentially 'or'd together
> collection of seteq
> -// instructions that compare a value against a constant, return the
> value being
> -// compared, and stick the constant into the Values vector.
> +// GatherConstantSetEQs - Given a potentially 'or'd together
> collection of
> +// icmp_eq instructions that compare a value against a constant,
> return the
> +// value being compared, and stick the constant into the Values
> vector.
> static Value *GatherConstantSetEQs(Value *V,
> std::vector<ConstantInt*> &Values){
> if (Instruction *Inst = dyn_cast<Instruction>(V))
> if (Inst->getOpcode() == Instruction::SetEQ) {
> if (ConstantInt *C =
> dyn_cast<ConstantInt>(Inst->getOperand(1))) {
> Values.push_back(C);
> return Inst->getOperand(0);
> } else if (ConstantInt *C =
> dyn_cast<ConstantInt>(Inst->getOperand(0))) {
> Values.push_back(C);
> return Inst->getOperand(1);
> }
>
>
> This code is dead because integer seteq can't exist anymore (it's
> icmp).
>
>
> + } else if (Inst->getOpcode() == Instruction::ICmp &&
> + cast<ICmpInst>(Inst)->getPredicate() == ICmpInst::ICMP_EQ) {
> + if (ConstantInt *C =
> dyn_cast<ConstantInt>(Inst->getOperand(1))) {
> + Values.push_back(C);
> + return Inst->getOperand(0);
> + } else if (ConstantInt *C =
> dyn_cast<ConstantInt>(Inst->getOperand(0))) {
> + Values.push_back(C);
> + return Inst->getOperand(1);
> + }
> + } else if (FCmpInst *FCI = dyn_cast<FCmpInst>(Inst)) {
> + FCmpInst::Predicate pred = FCI->getPredicate();
> + if (pred == FCmpInst::FCMP_ONE || pred == FCmpInst::FCMP_UNE) {
> + if (ConstantInt *C =
> dyn_cast<ConstantInt>(Inst->getOperand(1))) {
> + Values.push_back(C);
> + return Inst->getOperand(0);
> + } else if (ConstantInt *C =
> dyn_cast<ConstantInt>(Inst->getOperand(0))){
> + Values.push_back(C);
> + return Inst->getOperand(1);
> + }
> + }
>
>
> FCmpInst's can't have constantint operands, this code is dead.
Done.
>
>
> @@ -423,10 +445,30 @@ static Value *GatherConstantSetNEs(Value
> return Inst->getOperand(0);
> } else if (ConstantInt *C =
> dyn_cast<ConstantInt>(Inst->getOperand(0))) {
> Values.push_back(C);
> return Inst->getOperand(1);
> }
> + } else if (Inst->getOpcode() == Instruction::ICmp &&
> + cast<ICmpInst>(Inst)->getPredicate() ==
> ICmpInst::ICMP_NE) {
> + if (ConstantInt *C =
> dyn_cast<ConstantInt>(Inst->getOperand(1))) {
> + Values.push_back(C);
> + return Inst->getOperand(0);
> + } else if (ConstantInt *C =
> dyn_cast<ConstantInt>(Inst->getOperand(0))) {
> + Values.push_back(C);
> + return Inst->getOperand(1);
> + }
> + } else if (FCmpInst *FCI = dyn_cast<FCmpInst>(Inst)) {
> + FCmpInst::Predicate pred = FCI->getPredicate();
> + if (pred == FCmpInst::FCMP_ONE || pred == FCmpInst::FCMP_UNE) {
> + if (ConstantInt *C =
> dyn_cast<ConstantInt>(Inst->getOperand(1))) {
> + Values.push_back(C);
> + return Inst->getOperand(0);
> + } else if (ConstantInt *C =
> dyn_cast<ConstantInt>(Inst->getOperand(0))){
> + Values.push_back(C);
> + return Inst->getOperand(1);
> + }
> + }
>
>
>
>
> Likewise. Since you're handling integers, you can nuke the old
> SetCondInst code here.
Done.
>
>
>
>
>
>
>
>
> @@ -845,12 +887,11 @@ static bool HoistThenElseCodeToIf(Branch
> // identical order.
> BasicBlock *BB1 = BI->getSuccessor(0); // The true destination.
> BasicBlock *BB2 = BI->getSuccessor(1); // The false destination
>
>
>
> Instruction *I1 = BB1->begin(), *I2 = BB2->begin();
> - if (I1->getOpcode() != I2->getOpcode() || !I1->isIdenticalTo(I2) ||
> - isa<PHINode>(I1) || isa<InvokeInst>(I1))
> + if (!I1->isIdenticalTo(I2) || isa<PHINode>(I1) ||
> isa<InvokeInst>(I1))
> return false;
>
>
>
> // If we get here, we can hoist at least one instruction.
> BasicBlock *BIParent = BI->getParent();
>
>
>
>
>
> Why did you drop the opcode check? It is a fast reject path.
Because it is redundant with isIdenticalTo. I'll put it back to save the
function call.
>
> @@ -868,11 +909,11 @@ static bool HoistThenElseCodeToIf(Branch
> I2->replaceAllUsesWith(I1);
> BB2->getInstList().erase(I2);
>
>
>
> I1 = BB1->begin();
> I2 = BB2->begin();
> - } while (I1->getOpcode() == I2->getOpcode() &&
> I1->isIdenticalTo(I2));
> + } while (I1->isIdenticalTo(I2));
>
>
> likewise.
>
I put it back.
>
>
> Index: lib/Transforms/Utils/CloneFunction.cpp
> ===================================================================
> RCS file: /var/cvs/llvm/llvm/lib/Transforms/Utils/CloneFunction.cpp,v
> retrieving revision 1.33
> diff -t -d -u -p -5 -r1.33 CloneFunction.cpp
> --- lib/Transforms/Utils/CloneFunction.cpp 5 Nov 2006 19:31:28 -0000
> 1.33
> +++ lib/Transforms/Utils/CloneFunction.cpp 10 Dec 2006 23:26:50 -0000
> @@ -276,16 +276,19 @@ void PruningFunctionCloner::CloneBlock(c
>
>
>
> /// ConstantFoldMappedInstruction - Constant fold the specified
> instruction,
> /// mapping its operands through ValueMap if they are available.
> Constant *PruningFunctionCloner::
> ConstantFoldMappedInstruction(const Instruction *I) {
> - if (isa<BinaryOperator>(I) || isa<ShiftInst>(I)) {
> + if (isa<BinaryOperator>(I) || isa<ShiftInst>(I) || isa<CmpInst>(I))
> {
> if (Constant *Op0 =
> dyn_cast_or_null<Constant>(MapValue(I->getOperand(0),
>
> ValueMap)))
> if (Constant *Op1 =
> dyn_cast_or_null<Constant>(MapValue(I->getOperand(1),
>
> ValueMap)))
> - return ConstantExpr::get(I->getOpcode(), Op0, Op1);
> + return isa<CmpInst>(I) ?
> + ConstantExpr::getCompare(I->getOpcode(),
> + cast<CmpInst>(I)->getPredicate(), Op0, Op1) :
> + ConstantExpr::get(I->getOpcode(), Op0, Op1);
> return 0;
> }
>
>
>
> It seems more natural to split CmpInst out from binop/shift handling
> into a separate if, to eliminate the ternary operator.
Okay, I split it out.
>
>
>
>
> ===================================================================
> RCS file: /var/cvs/llvm/llvm/lib/Transforms/Scalar/Reassociate.cpp,v
> retrieving revision 1.68
> diff -t -d -u -p -5 -r1.68 Reassociate.cpp
> --- lib/Transforms/Scalar/Reassociate.cpp 7 Dec 2006 20:04:42 -0000
> 1.68
> +++ lib/Transforms/Scalar/Reassociate.cpp 10 Dec 2006 23:26:49 -0000
> @@ -754,12 +755,12 @@ void Reassociate::ReassociateBB(BasicBlo
> MadeChange = true;
> BI = NI;
> }
>
>
>
> // Reject cases where it is pointless to do this.
> - if (!isa<BinaryOperator>(BI) || BI->getType()->isFloatingPoint()
> ||
> - isa<PackedType>(BI->getType()))
> + if (!isa<BinaryOperator>(BI) || BI->getType()->isFloatingPoint()
> ||
> + isa<PackedType>(BI->getType()))
> continue; // Floating point ops are not associative.
>
>
>
>
> This change looks like an incorrect indentation change.
Yeah, I think I had a change in there and then backed it out. We
actually don't want to match CmpInst and now that CmpInst is not a
subclass of BinaryOperator, that's exactly what it gives us. I'll fix
the indentation.
>
>
>
>
>
>
> Index: lib/Transforms/Scalar/LoopUnswitch.cpp
> ===================================================================
> RCS file: /var/cvs/llvm/llvm/lib/Transforms/Scalar/LoopUnswitch.cpp,v
> retrieving revision 1.51
> diff -t -d -u -p -5 -r1.51 LoopUnswitch.cpp
> --- lib/Transforms/Scalar/LoopUnswitch.cpp 6 Dec 2006 17:46:33 -0000
> 1.51
> +++ lib/Transforms/Scalar/LoopUnswitch.cpp 10 Dec 2006 23:26:48 -0000
> @@ -486,11 +486,14 @@ static void EmitPreheaderBranchOnConditi
> Instruction *InsertPt) {
> // Insert a conditional branch on LIC to the two preheaders. The
> original
> // code is the true version and the new code is the false version.
> Value *BranchVal = LIC;
> if (!isa<ConstantBool>(Val)) {
> - BranchVal = BinaryOperator::createSetEQ(LIC, Val, "tmp",
> InsertPt);
> + if (Val->getType()->isFloatingPoint())
> + BranchVal = BinaryOperator::createSetEQ(LIC, Val, "tmp",
> InsertPt);
> + else
> + BranchVal = new ICmpInst(ICmpInst::ICMP_EQ, LIC, Val, "tmp",
> InsertPt);
> } else if (Val != ConstantBool::getTrue()) {
> // We want to enter the new loop when the condition is true.
> std::swap(TrueDest, FalseDest);
> }
>
>
>
> It's not obvious from immediate context, but this is always an integer
> comparison.
Okay, I deleted the FP case and removed the (now) unnecessary { and }
>
>
>
> diff -t -d -u -p -5 -r1.98 LoopStrengthReduce.cpp
> --- lib/Transforms/Scalar/LoopStrengthReduce.cpp 7 Dec 2006 01:30:31
> -0000 1.98
> +++ lib/Transforms/Scalar/LoopStrengthReduce.cpp 10 Dec 2006 23:26:48
> -0000
> @@ -1187,14 +1187,20 @@ void LoopStrengthReduce::OptimizeIndvars
> PHINode *SomePHI = cast<PHINode>(L->getHeader()->begin());
> BasicBlock *Preheader = L->getLoopPreheader();
> BasicBlock *LatchBlock =
> SomePHI->getIncomingBlock(SomePHI->getIncomingBlock(0) ==
> Preheader);
> BranchInst *TermBr =
> dyn_cast<BranchInst>(LatchBlock->getTerminator());
> + bool isICmp;
> if (!TermBr || TermBr->isUnconditional() ||
> - !isa<SetCondInst>(TermBr->getCondition()))
> + (!(isICmp = isa<ICmpInst>(TermBr->getCondition()) &&
> + !isa<SetCondInst>(TermBr->getCondition()))))
> return;
> - SetCondInst *Cond = cast<SetCondInst>(TermBr->getCondition());
> + Instruction *Cond;
> + if (isICmp)
> + Cond = cast<ICmpInst>(TermBr->getCondition());
> + else
> + Cond = cast<SetCondInst>(TermBr->getCondition());
>
>
>
>
> This code only cares about integer comparisons. Please change Cond to
> be ICmpInst*.
Okay. Done.
>
>
>
>
>
>
> @@ -1223,11 +1229,14 @@ void LoopStrengthReduce::OptimizeIndvars
> if (&*++BasicBlock::iterator(Cond) != (Instruction*)TermBr) {
> if (Cond->hasOneUse()) { // Condition has a single use, just
> move it.
> Cond->moveBefore(TermBr);
> } else {
> // Otherwise, clone the terminating condition and insert into
> the loopend.
> - Cond = cast<SetCondInst>(Cond->clone());
> + if (isICmp)
> + Cond = cast<ICmpInst>(Cond->clone());
> + else
> + Cond = cast<SetCondInst>(Cond->clone());
> Cond->setName(L->getHeader()->getName() + ".termcond");
> LatchBlock->getInstList().insert(TermBr, Cond);
>
>
>
> // Clone the IVUse, as the old use still exists!
> IVUsesByStride[*CondStride].addUser(CondUse->Offset, Cond,
>
>
> Likewise.
Done.
>
>
> @@ -1344,19 +1353,26 @@ void LoopStrengthReduce::runOnLoop(Loop
> // If all four cases above are true, then we can remove both
> the add and
> // the cann indvar.
> // FIXME: this needs to eliminate an induction variable even if
> it's being
> // compared against some value to decide loop termination.
> if (PN->hasOneUse()) {
> - BinaryOperator *BO =
> dyn_cast<BinaryOperator>(*(PN->use_begin()));
> - if (BO && BO->hasOneUse()) {
> - if (PN == *(BO->use_begin())) {
> + if (BinaryOperator *BO =
> dyn_cast<BinaryOperator>(*(PN->use_begin()))) {
> + if (BO->hasOneUse() && PN == *(BO->use_begin())) {
> DeadInsts.insert(BO);
> // Break the cycle, then delete the PHI.
> PN->replaceAllUsesWith(UndefValue::get(PN->getType()));
> SE->deleteInstructionFromRecords(PN);
> PN->eraseFromParent();
> }
> + } else if (CmpInst *CI =
> dyn_cast<CmpInst>(*(PN->use_begin()))) {
> + if (CI->hasOneUse() && PN == *(CI->use_begin())) {
> + DeadInsts.insert(CI);
> + // Break the cycle, then delete the PHI.
> + PN->replaceAllUsesWith(UndefValue::get(PN->getType()));
> + SE->deleteInstructionFromRecords(PN);
> + PN->eraseFromParent();
> + }
>
> Instead of cloning the code, how about something like:
>
>
> // FIXME: this needs to eliminate an induction variable even if
> it's being
> // compared against some value to decide loop termination.
> if (PN->hasOneUse()) {
> Instruction *BO = *PN->use_begin();
> if (isa<BinaryOperator>(BO) || isa<CmpInst>(BO)) {
> + if (BO->hasOneUse() && PN == *(BO->use_begin())) {
> DeadInsts.insert(BO);
> // Break the cycle, then delete the PHI.
> PN->replaceAllUsesWith(UndefValue::get(PN->getType()));
> SE->deleteInstructionFromRecords(PN);
> PN->eraseFromParent();
> }
Yes, that's much better.
>
>
>
> ===================================================================
> RCS file: /var/cvs/llvm/llvm/lib/Transforms/IPO/SimplifyLibCalls.cpp,v
> retrieving revision 1.74
> diff -t -d -u -p -5 -r1.74 SimplifyLibCalls.cpp
> --- lib/Transforms/IPO/SimplifyLibCalls.cpp 6 Dec 2006 17:46:32 -0000
> 1.74
> +++ lib/Transforms/IPO/SimplifyLibCalls.cpp 10 Dec 2006 23:26:42 -0000
> @@ -941,10 +941,16 @@ static bool IsOnlyUsedInEqualsZeroCompar
> if (User->getOpcode() == Instruction::SetNE ||
> User->getOpcode() == Instruction::SetEQ) {
> if (isa<Constant>(User->getOperand(1)) &&
> cast<Constant>(User->getOperand(1))->isNullValue())
> continue;
> + } else if (ICmpInst *IC = dyn_cast<ICmpInst>(User)) {
> + if ((IC->getPredicate() == ICmpInst::ICMP_NE ||
> + IC->getPredicate() == ICmpInst::ICMP_EQ) &&
> + isa<Constant>(IC->getOperand(1)) &&
> + cast<Constant>(IC->getOperand(1))->isNullValue())
> + continue;
> } else if (CastInst *CI = dyn_cast<CastInst>(User))
> if (CI->getType() == Type::BoolTy)
> continue;
> // Unknown instruction.
> return false;
>
>
> The FP case is dead here.
Removed.
>
>
> @@ -1769,13 +1775,13 @@ public:
> // isascii(c) -> (unsigned)c < 128
> Value *V = CI->getOperand(1);
> if (V->getType()->isSigned())
> V = CastInst::createInferredCast(V,
> V->getType()->getUnsignedVersion(),
> V->getName(), CI);
> - Value *Cmp = BinaryOperator::createSetLT(V,
> ConstantInt::get(V->getType(),
> -
> 128),
> - V->getName()+".isascii",
> CI);
> + Value *Cmp = new ICmpInst(ICmpInst::ICMP_ULT, V,
> + ConstantInt::get(V->getType(), 128),
> + V->getName()+".isascii", CI);
> if (Cmp->getType() != CI->getType())
> Cmp = CastInst::createInferredCast(
> Cmp, CI->getType(), Cmp->getName(), CI);
> CI->replaceAllUsesWith(Cmp);
> CI->eraseFromParent();
>
>
>
>
>
>
> You can drop the createInferredCast (which should have been a bitcast)
> now that compares are signless?
Yup, gone.
>
>
> diff -t -d -u -p -5 -r1.78 GlobalOpt.cpp
> --- lib/Transforms/IPO/GlobalOpt.cpp 7 Dec 2006 01:30:31 -0000 1.78
> +++ lib/Transforms/IPO/GlobalOpt.cpp 10 Dec 2006 23:26:41 -0000
> @@ -722,15 +722,53 @@ static GlobalVariable *OptimizeGlobalAdd
> std::vector<StoreInst*> Stores;
> while (!GV->use_empty())
> if (LoadInst *LI = dyn_cast<LoadInst>(GV->use_back())) {
> while (!LI->use_empty()) {
> Use &LoadUse = LI->use_begin().getUse();
> - if (!isa<SetCondInst>(LoadUse.getUser()))
> - LoadUse = RepValue;
> - else {
> + if (CmpInst *CI = dyn_cast<CmpInst>(LoadUse.getUser())) {
> + // Replace the cmp X, 0 with a use of the bool value.
> + Value *LV = new LoadInst(InitBool,
> InitBool->getName()+".val", CI);
> + InitBoolUsed = true;
> + switch (CI->getPredicate()) {
> + default: assert(0 && "Unknown ICmp Predicate!");
> + case FCmpInst::FCMP_OLT:
> + case FCmpInst::FCMP_ULT:
> + case ICmpInst::ICMP_ULT:
> + case ICmpInst::ICMP_SLT:
> + LV = ConstantBool::getFalse(); // X < null -> always
> false
> + break;
> + case FCmpInst::FCMP_ULE:
> + case FCmpInst::FCMP_OLE:
> + case FCmpInst::FCMP_OEQ:
> + case FCmpInst::FCMP_UEQ:
> + case ICmpInst::ICMP_ULE:
> + case ICmpInst::ICMP_SLE:
> + case ICmpInst::ICMP_EQ:
> + LV = BinaryOperator::createNot(LV, "notinit", CI);
> + break;
> + case ICmpInst::ICMP_NE:
> + case ICmpInst::ICMP_UGE:
> + case ICmpInst::ICMP_SGE:
> + case ICmpInst::ICMP_UGT:
> + case ICmpInst::ICMP_SGT:
> + case FCmpInst::FCMP_FALSE:
> + case FCmpInst::FCMP_OGT:
> + case FCmpInst::FCMP_OGE:
> + case FCmpInst::FCMP_ONE:
> + case FCmpInst::FCMP_ORD:
> + case FCmpInst::FCMP_UNO:
> + case FCmpInst::FCMP_UGT:
> + case FCmpInst::FCMP_UGE:
> + case FCmpInst::FCMP_UNE:
> + case FCmpInst::FCMP_TRUE:
> + break; // no change.
> + }
> + CI->replaceAllUsesWith(LV);
> + CI->eraseFromParent();
> + }
> + else if (SetCondInst *SCI =
> dyn_cast<SetCondInst>(LoadUse.getUser())) {
> // Replace the setcc X, 0 with a use of the bool value.
>
>
>
>
> This code is walking the use list of a global variable, so all the FP
> cases (including the SetCondInst case) are dead: only pointer compares
> are possible.
Yup, that's much cleaner.
>
>
>
>
> @@ -746,10 +784,12 @@ static GlobalVariable *OptimizeGlobalAdd
> break; // no change.
> }
> SCI->replaceAllUsesWith(LV);
> SCI->eraseFromParent();
> }
> + else
> + LoadUse = RepValue;
> }
> LI->eraseFromParent();
>
>
>
>
> I strongly prefer that you restore this code to its original order.
> Putting the short case at the bottom makes it harder to read the code,
> even if it makes dyn_cast more natural.
Okay. Done.
>
>
>
>
> Index: lib/Transforms/ExprTypeConvert.cpp
> ===================================================================
> RCS file: /var/cvs/llvm/llvm/lib/Transforms/ExprTypeConvert.cpp,v
> retrieving revision 1.115
> diff -t -d -u -p -5 -r1.115 ExprTypeConvert.cpp
> --- lib/Transforms/ExprTypeConvert.cpp 7 Dec 2006 01:30:31 -0000 1.115
> +++ lib/Transforms/ExprTypeConvert.cpp 10 Dec 2006 23:26:40 -0000
> @@ -456,10 +456,27 @@ static bool OperandConvertibleToType(Use
>
>
>
>
>
>
> + case Instruction::FCmp: {
> + FCmpInst::Predicate pred = cast<FCmpInst>(I)->getPredicate();
> + if (pred == FCmpInst::FCMP_OEQ || pred == FCmpInst::FCMP_UEQ ||
> + pred == FCmpInst::FCMP_ONE || pred == FCmpInst::FCMP_UNE) {
> + Value *OtherOp = I->getOperand((V == I->getOperand(0)) ? 1 :
> 0);
> + return ExpressionConvertibleToType(OtherOp, Ty, CTMap, TD);
> + }
> + return false;
> + }
> case Instruction::SetEQ:
> case Instruction::SetNE: {
> Value *OtherOp = I->getOperand((V == I->getOperand(0)) ? 1 : 0);
> return ExpressionConvertibleToType(OtherOp, Ty, CTMap, TD);
> }
>
>
> This code (FCmp and SetEQ/SetNE) is dead, only integer types can get
> this far.
Okay. Done.
>
>
> @@ -721,10 +738,37 @@ static void ConvertOperandToType(User *U
> ...
> Res->setOperand(!OtherIdx, NewVal)
> ;+ case Instruction::FCmp: {
> + FCmpInst::Predicate pred = cast<FCmpInst>(I)->getPredicate();
> + if (pred == FCmpInst::FCMP_OEQ || pred == FCmpInst::FCMP_UEQ ||
> + pred == FCmpInst::FCMP_ONE || pred == FCmpInst::FCMP_UNE) {
> + Res = new FCmpInst(pred, Dummy, Dummy, Name);
> + VMC.ExprMap[I] = Res; // Add node to expression eagerly
> + unsigned OtherIdx = (OldVal == I->getOperand(0)) ? 1 : 0;
> + Value *OtherOp = I->getOperand(OtherIdx);
> + Res->setOperand(!OtherIdx, NewVal);
> + Value *NewOther = ConvertExpressionToType(OtherOp, NewTy,
> VMC, TD);
> + Res->setOperand(OtherIdx, NewOther);
> + }
> + break;
> + }
> case Instruction::Shl:
> case Instruction::LShr:
> case Instruction::AShr:
> assert(I->getOperand(0) == OldVal);
> Res = new ShiftInst(cast<ShiftInst>(I)->getOpcode(), NewVal,
>
>
> Likewise.
Done.
>
>
>
>
> -Chris
>
>
More information about the llvm-commits
mailing list