[llvm-commits] [llvm] r149481 - in /llvm/trunk: include/llvm/ include/llvm/Analysis/ lib/Analysis/ lib/Bitcode/Writer/ lib/CodeGen/SelectionDAG/ lib/ExecutionEngine/Interpreter/ lib/Target/CBackend/ lib/Target/CppBackend/ lib/Transforms/IPO/ lib/Transforms/InstCombine/ lib/Transforms/Scalar/ lib/Transforms/Utils/ lib/VMCore/ tools/llvm-diff/
Duncan Sands
baldrick at free.fr
Thu Feb 2 01:58:55 PST 2012
Hi Stepan,
> --- llvm/trunk/include/llvm/Instructions.h (original)
> +++ llvm/trunk/include/llvm/Instructions.h Wed Feb 1 01:49:51 2012
> @@ -24,6 +24,7 @@
> #include "llvm/ADT/SmallVector.h"
> #include "llvm/Support/ErrorHandling.h"
> #include<iterator>
> +#include<limits.h>
please don't include this. You can just use ~0U.
>
> namespace llvm {
>
> @@ -2467,6 +2468,9 @@
> protected:
> virtual SwitchInst *clone_impl() const;
> public:
> +
> + enum { ErrorIndex = UINT_MAX };
Here you can use ~0U rather than UINT_MAX.
> + /// resolveSuccessorIndex - Converts case index to index of its successor
> + /// index in TerminatorInst successors collection.
This comment is kind of obscure. There are too many uses of "index" flying
around. What is a successor in this context? I think I finally understood
that: a switch instruction has a number of edges coming out of it, and this
returns the edge index for the case. It would be nice if (like in Ada) you
could declare the different kinds of indices to have different types so that
they can't accidentally be confused. In fact you could make a "case index"
be opaque by creating a new CaseIndex class, and having indices be of that
class type.
> + /// If CaseIndex == ErrorIndex, "default" successor will returned then.
Why this behaviour? Is it a good idea to have ErrorIndex mean: index of
the default case? Because then it doesn't represent an error any more!
I think you should either rename ErrorIndex to DefaultIndex or change the
logic so that using ErrorIndex is an error, i.e. triggers an assert.
> + /// resolveCaseIndex - Converts index of successor in TerminatorInst
> + /// collection to index of case that corresponds to this successor.
You didn't say it returns ErrorIndex if at the first case. And why does
it do that? Shouldn't an assertion fire then? See comments above.
> + unsigned resolveCaseIndex(unsigned SuccessorIndex) const {
> + assert(SuccessorIndex< getNumSuccessors()&&
> + "Successor index # out of range!");
> + return SuccessorIndex != 0 ? SuccessorIndex - 1 : ErrorIndex;
> }
>
> /// findCaseDest - Finds the unique case value for a given successor. Returns
> @@ -2554,6 +2585,22 @@
> setOperand(idx*2+1, (Value*)NewSucc);
> }
>
> + /// Resolves successor for idx-th case.
> + /// Use getCaseSuccessor instead of TerminatorInst::getSuccessor,
> + /// since internal SwitchInst organization of operands/successors is
> + /// hidden and may be changed in any moment.
I don't understand the point of this "Use getCaseSuccessor ..." comment.
There are perfectly legitimate uses of TerminatorInst::getSuccessor, i.e.
those which don't give a damn about case indices. So the comment is wrong
as it stands. Otherwise the only way to get things wrong is if use a case
index as a successor index. To prevent this easy accident you need more than
a comment, you need a way to make it impossible to confuse the types (see my
comment on introducing a class for this above).
> + /// Set new successor for idx-th case.
> + /// Use setCaseSuccessor instead of TerminatorInst::setSuccessor,
> + /// since internal SwitchInst organization of operands/successors is
> + /// hidden and may be changed in any moment.
Likewise.
> --- llvm/trunk/lib/Analysis/SparsePropagation.cpp (original)
> +++ llvm/trunk/lib/Analysis/SparsePropagation.cpp Wed Feb 1 01:49:51 2012
> @@ -195,7 +195,8 @@
> return;
> }
>
> - Succs[SI.findCaseValue(cast<ConstantInt>(C))] = true;
> + unsigned CCase = SI.findCaseValue(cast<ConstantInt>(C));
> + Succs[SI.resolveSuccessorIndex(CCase)] = true;
If the case is not found, kaboom! Use up all memory and die due to accessing
element UINT_MAX... Is this possible? Previously, did you get the default
case (0) here if not found?
> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp Wed Feb 1 01:49:51 2012
> @@ -2209,7 +2209,7 @@
>
> CaseRange LHSR(CR.Range.first, Pivot);
> CaseRange RHSR(Pivot, CR.Range.second);
> - Constant *C = Pivot->Low;
> + const Constant *C = Pivot->Low;
LLVM isn't into const qualifiers.
> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.h Wed Feb 1 01:49:51 2012
> @@ -130,13 +130,13 @@
> /// Case - A struct to record the Value for a switch case, and the
> /// case's target basic block.
> struct Case {
> - Constant* Low;
> - Constant* High;
> + const Constant *Low;
> + const Constant *High;
Likewise.
> MachineBasicBlock* BB;
> uint32_t ExtraWeight;
>
> Case() : Low(0), High(0), BB(0), ExtraWeight(0) { }
> - Case(Constant* low, Constant* high, MachineBasicBlock* bb,
> + Case(const Constant *low, const Constant *high, MachineBasicBlock *bb,
Likewise.
> --- llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp (original)
> +++ llvm/trunk/lib/Transforms/IPO/GlobalOpt.cpp Wed Feb 1 01:49:51 2012
> @@ -2455,7 +2455,8 @@
> ConstantInt *Val =
> dyn_cast<ConstantInt>(getVal(Values, SI->getCondition()));
> if (!Val) return false; // Cannot determine.
> - NewBB = SI->getSuccessor(SI->findCaseValue(Val));
> + unsigned ValTISucc = SI->resolveSuccessorIndex(SI->findCaseValue(Val));
> + NewBB = SI->getSuccessor(ValTISucc);
Does this resolve to the default successor if the case is not found?
> --- llvm/trunk/lib/Transforms/Scalar/LoopUnswitch.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/LoopUnswitch.cpp Wed Feb 1 01:49:51 2012
> @@ -1118,14 +1118,15 @@
> if (SI == 0 || !isa<ConstantInt>(Val)) continue;
>
> unsigned DeadCase = SI->findCaseValue(cast<ConstantInt>(Val));
> - if (DeadCase == 0) continue; // Default case is live for multiple values.
> + // Default case is live for multiple values.
> + if (DeadCase == SwitchInst::ErrorIndex) continue;
Another reason why ErrorIndex should probably be DefaultIndex.
> --- llvm/trunk/lib/Transforms/Scalar/SCCP.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/SCCP.cpp Wed Feb 1 01:49:51 2012
> @@ -564,7 +564,7 @@
> return;
> }
>
> - Succs[SI->findCaseValue(CI)] = true;
> + Succs[SI->resolveSuccessorIndex(SI->findCaseValue(CI))] = true;
Kaboom on the default case?
> @@ -624,9 +624,9 @@
> return !SCValue.isUndefined();
>
> // Make sure to skip the "default value" which isn't a value
> - for (unsigned i = 1, E = SI->getNumSuccessors(); i != E; ++i)
> - if (SI->getSuccessorValue(i) == CI) // Found the taken branch.
> - return SI->getSuccessor(i) == To;
> + for (unsigned i = 0, E = SI->getNumCases(); i != E; ++i)
> + if (SI->getCaseValue(i) == CI) // Found the taken branch.
> + return SI->getCaseSuccessor(i) == To;
Doesn't SwitchInst define a lookup method that does this?
> --- llvm/trunk/lib/Transforms/Utils/Local.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/Local.cpp Wed Feb 1 01:49:51 2012
> @@ -106,22 +106,20 @@
> // If we are switching on a constant, we can convert the switch into a
> // single branch instruction!
> ConstantInt *CI = dyn_cast<ConstantInt>(SI->getCondition());
> - BasicBlock *TheOnlyDest = SI->getSuccessor(0); // The default dest
> + BasicBlock *TheOnlyDest = SI->getDefaultDest(); // The default dest
The comment "// The default dest" is no longer useful.
> --- llvm/trunk/lib/Transforms/Utils/LowerExpectIntrinsic.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/LowerExpectIntrinsic.cpp Wed Feb 1 01:49:51 2012
> @@ -76,11 +76,14 @@
> unsigned caseNo = SI->findCaseValue(ExpectedValue);
> std::vector<Value *> Vec;
> unsigned n = SI->getNumCases();
> - Vec.resize(n + 1); // +1 for MDString
> + Vec.resize(n + 1 + 1); // +1 for MDString and +1 for default case
>
> Vec[0] = MDString::get(Context, "branch_weights");
> + Vec[1] = ConstantInt::get(Int32Ty, SwitchInst::ErrorIndex == caseNo ?
> + LikelyBranchWeight : UnlikelyBranchWeight);
> for (unsigned i = 0; i< n; ++i) {
> - Vec[i + 1] = ConstantInt::get(Int32Ty, i == caseNo ? LikelyBranchWeight : UnlikelyBranchWeight);
> + Vec[i + 1 + 1] = ConstantInt::get(Int32Ty, i == caseNo ?
> + LikelyBranchWeight : UnlikelyBranchWeight);
> }
>
> MDNode *WeightsNode = llvm::MDNode::get(Context, Vec);
This seems to contain a bug fix/behaviour change, as such it should not have
been included in this patch. That said, it's there now so it might as well be
left there since it seems correct to me. Once more ErrorIndex is not indicating
an error...
> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Wed Feb 1 01:49:51 2012
> @@ -2007,8 +2010,10 @@
>
> // Find the relevant condition and destinations.
> Value *Condition = Select->getCondition();
> - BasicBlock *TrueBB = SI->getSuccessor(SI->findCaseValue(TrueVal));
> - BasicBlock *FalseBB = SI->getSuccessor(SI->findCaseValue(FalseVal));
> + unsigned TrueCase = SI->findCaseValue(TrueVal);
> + unsigned FalseCase = SI->findCaseValue(FalseVal);
> + BasicBlock *TrueBB = SI->getSuccessor(SI->resolveSuccessorIndex(TrueCase));
> + BasicBlock *FalseBB = SI->getSuccessor(SI->resolveSuccessorIndex(FalseCase));
Since this idiom occurs a lot, how about adding a method for it to SwitchInst?
> @@ -2616,8 +2621,10 @@
> // Remove dead cases from the switch.
> for (unsigned I = 0, E = DeadCases.size(); I != E; ++I) {
> unsigned Case = SI->findCaseValue(DeadCases[I]);
> + assert(Case != SwitchInst::ErrorIndex&&
> + "Case was not found. Probably mistake in DeadCases forming.");
As it actually wrong to get the default case here?
> // Prune unused values from PHI nodes.
> - SI->getSuccessor(Case)->removePredecessor(SI->getParent());
> + SI->getCaseSuccessor(Case)->removePredecessor(SI->getParent());
> SI->removeCase(Case);
> }
>
Ciao, Duncan.
More information about the llvm-commits
mailing list