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

[Eli Friedman]

r140803.

-Eli

On Thu, Sep 29, 2011 at 3:57 AM, Stepan Dyatkovskiy <stpworld at narod.ru> wrote:
> 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
>
>