[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/
Stepan Dyatkovskiy
STPWORLD at narod.ru
Wed Feb 15 02:41:55 PST 2012
Hi, Duncan. Please find the patch in attachment with respective changes.
I implemented CaseIterator and it solves almost all described issues. Base iterator class is implemented as a template since it may be initialized either from "const SwitchInst*" or from "SwitchInst*".
ConstCaseIt is just a read-only iterator.
CaseIt is read-writer iterator; it allows to change case successor and case value.
This approach allows totally remove resolveXXXX methods. It done automatically inside the iterator's getters.
Main way of iterator usage looks like this:
SwitchInst *SI = ... // something
for (SwitchInst::CaseIt i = SI->caseBegin(), e = SI->caseEnd(); i != e; ++i) {
BasicBlock *BB = i.getCaseSuccessor();
ConstantInt *V = i.getCaseValue();
// Do something.
}
If you want to convert case number to TerminatorInst successor index, just use getSuccessorIndex iterator's method.
If you want initialize iterator from TerminatorInst successor index, use CaseIt::fromSuccessorIndex(...) method.
I also attached patches for llvm-clients affected: klee and clang.
-Stepan.
13.02.2012, 11:38, "Stepan Dyatkovskiy" <stpworld at narod.ru>:
> Hi, Duncan.
>
>>> +#include<limits.h>
>> please don't include this. You can just use ~0U.
>
> OK.
>
>>> + /// 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.
>
> Hm... I thought about that too. Did you offer to implement some kind of
> iterators?
>
>>> + /// 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.
>
> Yes. DefaultIndex sounds much more better. I even propose to call it
> DefaultCase, since it doesn't index anything. Its like a some kind of
> unreachable numbers (infinity, or sqrt(-1) for real numbers), in short
> its not an eigenvalue.
>
>>> + /// 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.
>
> You right. If we replace ErrorIndex with DefaultCase, we got more
> logical behaviour:
> We have default successor and cases successors only. So if we meet some
> successor that are not belongs to any case, that means we got default
> successor. Also as you noticed I'll update the comment and add
> description what will returned for Successor with zero index.
>
>>> + /// 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).
>
> I think, iterators will solved that. In comment I asked do not mix
> TerminatorInst indexing with cases indexing. Of course if you want to
> use switch instruction as TerminatorInst - there is no crime to use
> get/setSuccessor methods. But again - iterators shoot this issue.
>
>>> - 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?
>
> resolveSuccessorIndex remaps CCase == UINT_MAX to default successor index.
>
>>> --- 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.
>
> I've already noticed it. I need to do that instead.
> SelectionDAGBuilder::visitSwitch works with "const SwitchInst &SI". So
> in that case I can work with "const ConstantInt
> SwitchInst::getCaseValue(unsigned) const" only. I kept this method
> declaration without changes. Do you propose to change it return value
> with "ConstaintInt*" instead of "const ConstantInt*" ?
> IMHO, looking LLVM sources I also noticed that there are some confusion
> relative to this subject. Just compare two method prototypes (I kept
> them unchanged):
> const ConstantInt *getCaseValue(unsigned i) const
> and
> ConstantInt *getSuccessorValue(unsigned idx) const
> So, looking on that I was confused too and decided to use "const".
> BTW, the last prototype is unused and should be removed.
>
> I'll apply the changes as community wishes. But IMHO it is better to use
> "const" modifier wherever it possible. In another case, this approach
> like a some kind of infection will removed all "const" modifiers in
> whole LLVM and its clients.
>
>>> --- 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?
>
> Yes.
>
>>> --- 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?
>
> Not. Default successor will resolved.
>
>>> @@ -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?
>
> Yes. Sorry for stupid change. Of course we can use findCaseValue here.
>
>>> --- 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.
>
> ok.
>
>>> --- 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...
>
> I tried to keep semantics the same. Leaving that without changes means
> to change the semantics. Here we need to set Likely/Unlikey weights for
> DefaultCase and for all other Cases. Since getNumCases/getCaseValue is
> no longer enumerates DefaultCase, I need to write it explicitly outside
> the cycle.
>
>>> --- 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?
>
> Good idea. I can add something like a "BasicBlock
> *resolveSuccessor(CaseIndex &idx)". I will return either case succesor
> or default successor if idx == DefaultCase.
>
>>> @@ -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?
>
> Even if it dead we can't remove it here. Also algorithm implemented here
> said that its impossible. We analyse the condition bits and we calculate
> the bits that MUST be in value. Else the case value will never equals to
> condition - dead case. Current algorithm implementation will never
> detected that the default case is dead.
>
> Summary:
> 1. As you proposed I'll implement CaseIterator. It allows as to solve
> problem with mixing case indices with TerminatorInst indices, and with
> operand's indices.
> 2. I can replace "const ConstantInt*" with "ConstantInt*" in selection
> DAG. But I have a doubts relative to this change.
> 3. I'll implement "BasicBlock *resolveSuccessor(CaseIndex &idx)".
> 4. I also will remove getSuccessorValue, since it lost its semantics and
> unused.
>
> What do you think about it?
>
> -Stepan.
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
A non-text attachment was scrubbed...
Name: si-cleanup-2.patch
Type: application/octet-stream
Size: 44265 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120215/1c221529/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: si-cleanup-2-klee.patch
Type: application/octet-stream
Size: 2648 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120215/1c221529/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: si-cleanup-2-clang.patch
Type: application/octet-stream
Size: 684 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120215/1c221529/attachment-0002.obj>
More information about the llvm-commits
mailing list