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

[Stepan Dyatkovskiy]

> 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)?

> 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");>
> The rest of the changes look fine.
>
> -Eli