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

[Evan Cheng]

LGTM

Evan

On Nov 5, 2012, at 5:20 PM, liangh at codeaurora.org wrote:

> 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
>>>> 
>>>> 
>>> 
>>> 
>> 
>> 
> <0001-Thumb2-is-using-a-32-bit-instruction-encoded-as-a-pr.patch><Thumb2SizeReduction.cpp>