[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]

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.