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

[Matthew Gretton-Dann <matthew dot gretton-dann at arm dot com>]

I'm not a maintainer so can't approve this patch - but I do have some
comments on it:

On Thu, Mar 29, 2012 at 10:02:14PM +0100, Meador Inge wrote:
> Hi,
> 
> This patch changes GAS to emit the A2 encoding for PUSH/POP instructions
> with a single register.  This case is specified by the ARMARM: A8.8.132,
> A8.8.133 [1].  The A2 encoding is allowed on the following architecture
> versions: ARMv4*, ARMv5T*, ARMv6*, and ARMv7.
> 
> Tested with arm-none-eabi configuration.  No regressions.
> 
> OK?
> 
> P.S. If this is OK, then can someone commit for me?  I don't have write
> access.
> 
> [1] http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0406c/index.html
> 
> gas/
> 2012-03-29  Jie Zhang  <jie@codesourcery.com>
>             Meador Inge  <meadori@codesourcery.com>
> 
> 	* config/tc-arm.c (only_one_reg_in_list): New function.
> 	(do_ldmstm): Use a different encoding when pushing or poping
> 	a single register.
> 
> gas/testsuite/
> 2012-03-29  Jie Zhang  <jie@codesourcery.com>
>             Meador Inge  <meadori@codesourcery.com>
> 
> 	* gas/arm/push-pop.d: New testcase.
> 	* gas/arm/push-pop.s: New testcase.
> 
> 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;

?

(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?


-- 
Matthew Gretton-Dann
Principal Engineer, PD Software, ARM Ltd.