[llvm-commits] patch: THUMB2 support for PUSH a single reg

[liangh at codeaurora.org]

HiEvan,

1) I've updated the patch according to all the feedbacks.
2) FYI, I attached the updated file Thumb2SizeReduction.cpp.
Please review.

Thanks.
-Liang

>
> On Oct 23, 2012, at 3:35 PM, liangh at codeaurora.org wrote:
>
>> Hi Evan,
>>
>> Please see below inlined answers.
>>
>> Thanks.
>> -Liang
>>
>>> 1: Stylistic nitpick:
>>> +  if(!Skip){
>>> Please make sure there is a space between 'if' and '('.
>>
>> Will fix it.
>>
>>>
>>> 2.
>>> +  case ARM::t2STR_PRE:{
>>> +    if (MI->getOperand(2).getReg() != ARM::SP)
>>> +      return false;
>>>
>>> Is this correct? Does it need to check the offset?
>>
>> Yes. We are not targeting all t2STR_PRE. We are targeting "PUSH", so we
>> only care about instructions like "str r7, [sp, #-4]!" who has SP as a
>> base address.
>>
>>>
>>> 3.
>>>
>>> +    Skip=true;
>>> +    break;
>>> +  }
>>>   case ARM::t2LDMIA_UPD:
>>>   case ARM::t2LDMDB_UPD:
>>>   case ARM::t2STMIA_UPD:
>>> @@ -430,46 +446,48 @@
>>> Thumb2SizeReduce::ReduceLoadStore(MachineBasicBlock
>>> &MBB, MachineInstr *MI,
>>>   }
>>>   }
>>>
>>> -  unsigned OffsetReg = 0;
>>> -  bool OffsetKill = false;
>>> -  if (HasShift) {
>>> -    OffsetReg  = MI->getOperand(2).getReg();
>>> -    OffsetKill = MI->getOperand(2).isKill();
>>> +  if(!Skip){
>>>
>>> Why not use an early exit instead of the nested condition?
>>
>> That was to share the common code after this skipped section. To my
>> understanding, copying those lines and exit early is not better; because
>> people would have to change two places if they want to change these
>> shared
>> code before return, and they might have concern on touching the code
>> located in a special case. But if you think that is a cleanner way I'm
>> open to make the change as you suggested.
>
> It's hard to see that from just the patch. In addition to the patch,
> please also include a copy of the updated Thumb2SizeReduction.cpp.
>
> Thanks,
>
> Evan
>
>>
>>>
>>> Evan
>>>
>>> On Oct 19, 2012, at 4:02 PM, liangh at codeaurora.org wrote:
>>>
>>>> Hi,
>>>>
>>>> Currently, LLVM doesn't generate PUSH instructions for THUMB2 when a
>>>> single register is pushed.
>>>> For example, it issues "str r7, [sp, #-4]!" but not "push {r7}".
>>>>
>>>> This patch adds THUMB2 support for PUSH instruction with a single
>>>> register. So that a real PUSH instruction will be generated for this
>>>> case.
>>>> I.e.: str r7, [sp, #-4]! => push {r7}
>>>>
>>>> Could you please review the attached patch?
>>>> Thanks.
>>>>
>>>> -Liang<0001-Add-Thumb2-support-for-PUSH-instruction-with-a-singl.patch>_______________________________________________
>>>> llvm-commits mailing list
>>>> llvm-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>
>>
>>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Thumb2-is-using-a-32-bit-instruction-encoded-as-a-pr.patch
Type: application/octet-stream
Size: 6990 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121105/0804ff81/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Thumb2SizeReduction.cpp
Type: application/octet-stream
Size: 34456 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121105/0804ff81/attachment-0001.obj>