[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
Mon Mar 5 04:59:01 PST 2012
ping.
15.02.2012, 14:41, "Stepan Dyatkovskiy" <STPWORLD at narod.ru>:
> 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
More information about the llvm-commits
mailing list