[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