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

[Stepan Dyatkovskiy]

Hi,
Please find the fixed patch.

Regards,
Stepan.

Eli Friedman wrote:
> 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: cr-cleanup.patch
Type: text/x-patch
Size: 9471 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20110928/d22ab417/attachment.bin>