[llvm-commits] [LLVM, Switch case ranges] Cleanup patch before working on switch case ranges

[Eli Friedman]

On Wed, Sep 28, 2011 at 12:03 AM, Stepan Dyatkovskiy <stpworld at narod.ru> wrote:
>> On Tue, Sep 27, 2011 at 11:32 AM, Stepan Dyatkovskiy<stpworld at narod.ru>
>>  wrote:
>>>
>>> Hi,
>>>
>>> Please find attached the clean-up patch for the SwitchInst internals
>>> before
>>> working on switch case ranges
>>> (http://llvm.org/bugs/show_bug.cgi?id=1255).
>>> Patches for switch case ranges will follow.
>>
>> I'm not sure about the changes to
>> lib/Target/CppBackend/CPPBackend.cpp... it looks like it ends up doing
>> extra work.
>
> void CppWriter::printInstruction has mixed implementation inside. For some
> instructions opNames array is used:
>
>    const BranchInst* br = cast<BranchInst>(I);
>    Out << "BranchInst::Create(" ;
>    if (br->getNumOperands() == 3) {
>      Out << opNames[2] << ", "
>          << opNames[1] << ", "
>          << opNames[0] << ", ";
>
>    } else if (br->getNumOperands() == 1) {
>      Out << opNames[0] << ", ";
>    } else {
>      error("Branch with 2 operands?");
>    }
>
> But there are also instructions for which its own interface is used:
>  const InvokeInst* inv = cast<InvokeInst>(I);
>  // ...
>  // some code here
>  // ...
>  Out << "InvokeInst *" << iName << " = InvokeInst::Create("
>        << getOpName(inv->getCalledFunction()) << ", "
>        << getOpName(inv->getNormalDest()) << ", "
>        << getOpName(inv->getUnwindDest()) << ", "
>        << iName << "_params, \"";
>
> SwitchInst has unclear usage of operands array. Each even operand is a Case
> Value and it has ConstantInt type. And each odd operand is a case
> destination and it has BasicBlock type. So probably it is better to use
> SwitchInst interface here (like for InvokeInst)?

Okay; this isn't really performance-sensitive code.

>> Probably not a big deal, though.
>>
>> +        for (unsigned i = 1; i<  NumCases; ++i) {
>> +          ConstantInt* CaseVal = SI.getCaseValue(i);
>> +          Constant* NewCaseVal =
>> ConstantExpr::getSub(cast<Constant>(CaseVal),
>> +              AddRHS);
>> +          assert(isa<ConstantInt>(NewCaseVal)&&
>> +              "Result of expression should be constant");
>>
>> Slightly strange indentation here.
>
> Did you mean something like this?
>
> +        for (unsigned i = 1; i<  NumCases; ++i) {
> +          ConstantInt* CaseVal = SI.getCaseValue(i);
> +          Constant* NewCaseVal =
> ConstantExpr::getSub(cast<Constant>(CaseVal),
> +                                                      AddRHS);
> +          assert(isa<ConstantInt>(NewCaseVal)&&
> +                 "Result of expression should be constant");>

Yes.

-Eli