[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 Mar 7 00:40:12 PST 2012
Hi Duncan,
Is this patch OK for you?
I'm going to commit that.
-Stepan.
05.03.2012, 16:59, "Stepan Dyatkovskiy" <STPWORLD at narod.ru>:
> 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
>
> _______________________________________________
> 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/20120307/17427c89/attachment.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/20120307/17427c89/attachment-0001.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/20120307/17427c89/attachment-0002.obj>
More information about the llvm-commits
mailing list