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

[Stepan Dyatkovskiy]

ping...

Thanks,
Stepan

Stepan Dyatkovskiy wrote:
> 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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits