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

[Stepan Dyatkovskiy]

Thanks!

Regards,
Stepan

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