Re: [PATCH] gas: Emit A2 encoding for ARM PUSH/POP with single register

[Meador Inge <meadori at codesourcery dot com>]

On 03/30/2012 04:01 AM, Matthew Gretton-Dann wrote:

>> diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
>> index 585f78e..826cf62 100644
>> --- a/gas/config/tc-arm.c
>> +++ b/gas/config/tc-arm.c
>> @@ -7795,11 +7795,30 @@ do_it (void)
>>      }
>>  }
>>  
>> +/* If there is only one register in the register list, return the register
>> +   number of that register.  Otherwise return -1.  */
>> +static int
>> +only_one_reg_in_list (int range)
>> +{
>> +  int i;
>> +
>> +  if (range <= 0 || range > 0xffff
>> +      || (range & (range - 1)) != 0)
>> +    return -1;
>> +
>> +  for (i = 0; i <= 15; i++)
>> +    if (range & (1 << i))
>> +      break;
>> +
>> +  return i;
>> +}
>> +
> 
> Would this be better as something like:
> 
>    int i = ffs (range);
>    return (i > 15 || range != (1 << i)) ? -1 : i;
> 
> ?

Yes, that is much nicer.  'ffs' numbers bits from 1, though, so:

   int i = ffs (range) - 1;
   return (i > 15 || range != (1 << i)) ? -1 : i;

> (See do_t_ldmstm which tries to do the same calculation).
> 
>>  static void
>>  do_ldmstm (void)
>>  {
>>    int base_reg = inst.operands[0].reg;
>>    int range = inst.operands[1].imm;
>> +  int one_reg;
>>  
>>    inst.instruction |= base_reg << 16;
>>    inst.instruction |= range;
>> @@ -7832,6 +7851,26 @@ do_ldmstm (void)
>>  	    as_warn (_("if writeback register is in list, it must be the lowest reg in the list"));
>>  	}
>>      }
>> +
>> +  /* When POP or PUSH only one register, we have to use different encodings.  */
>> +  one_reg = only_one_reg_in_list (range);
>> +  if (one_reg >= 0)
>> +    {
>> +      if ((inst.instruction & 0xfff0000) == 0x8bd0000)
>> +	{
>> +	  inst.instruction &= 0xf0000000;
>> +	  inst.instruction |= 0x49d0004;
>> +	}
>> +      else if ((inst.instruction & 0xfff0000) == 0x92d0000)
>> +	{
>> +	  inst.instruction &= 0xf0000000;
>> +	  inst.instruction |= 0x52d0004;
>> +	}
>> +      else
>> +	return;
> 
> Can you factor these tests on inst.instruction into separate functions so
> that it becomes clearer what we are testing for?  One is obviously a push,
> and the other a pop - but which way round?
> 
> Also can we have some #define'd cosntants used instead of the magic
> numbers so that it is clear what you are changing the instruction into?

I think use of macros and some better commenting will fix both of the above
issues.

Thanks for the review.  v2 patch on the way.

-- 
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software