Re: [PATCH 3/4] [ARM] Refactor pseudo register numbering

[Alan Hayward via Gdb-patches <gdb-patches@sourceware.org>]

This is similar to AArch64 target, so mostly Lgtm...

> On 5 Oct 2021, at 15:45, Luis Machado <luis.machado@linaro.org> wrote:
> 
> The pseudo register handling for ARM uses some hardcoded constants to
> determine types and names.  In preparation to the upcoming MVE support
> patch (that will add another pseudo register), this patch refactors and
> reorganizes things in order to simplify handling of future pseudo registers.
> 
> We keep track of the first pseudo register number in a group and the number of
> pseudo registers in that group.
> 
> Right now we only have the S and Q pseudo registers.

Renaming NEON to Q makes sense.

Why not rename VFP to S?


> ---
> gdb/arm-tdep.c | 139 +++++++++++++++++++++++++++++++++++--------------
> gdb/arm-tdep.h |  12 ++++-
> 2 files changed, 111 insertions(+), 40 deletions(-)
> 
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index 2a6bfb1b3f7..13bce202585 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -4122,20 +4122,57 @@ arm_neon_quad_type (struct gdbarch *gdbarch)
>   return tdep->neon_quad_type;
> }
> 
> +/* Return true if REGNUM is a Q pseudo register.  Return false
> +   otherwise.
> +
> +   REGNUM is the raw register number and not a pseudo-relative register
> +   number.  */
> +
> +static bool
> +is_q_pseudo (struct gdbarch *gdbarch, int regnum)
> +{
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  /* Q pseudo registers are available for NEON (Q0~Q15).  */
> +  if (tdep->have_q_pseudos
> +      && regnum >= tdep->q_pseudo_base
> +      && regnum < (tdep->q_pseudo_base + tdep->q_pseudo_count))
> +    return true;
> +
> +  return false;
> +}
> +
> +/* Return true if REGNUM is a VFP pseudo register.  Return false
> +   otherwise.
> +
> +   REGNUM is the raw register number and not a pseudo-relative register
> +   number.  */
> +
> +static bool
> +is_vfp_pseudo (struct gdbarch *gdbarch, int regnum)
> +{
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  if (tdep->have_vfp_pseudos
> +      && regnum >= tdep->vfp_pseudo_base
> +      && regnum < (tdep->vfp_pseudo_base + tdep->vfp_pseudo_count))
> +    return true;
> +
> +  return false;
> +}
> +
> /* Return the GDB type object for the "standard" data type of data in
>    register N.  */
> 
> static struct type *
> arm_register_type (struct gdbarch *gdbarch, int regnum)
> {
> -  int num_regs = gdbarch_num_regs (gdbarch);
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> 
> -  if (gdbarch_tdep (gdbarch)->have_vfp_pseudos
> -      && regnum >= num_regs && regnum < num_regs + 32)
> +  if (is_vfp_pseudo (gdbarch, regnum))
>     return builtin_type (gdbarch)->builtin_float;
> 
> -  if (gdbarch_tdep (gdbarch)->have_neon_pseudos
> -      && regnum >= num_regs + 32 && regnum < num_regs + 32 + 16)
> +  if (is_q_pseudo (gdbarch, regnum))
>     return arm_neon_quad_type (gdbarch);
> 
>   /* If the target description has register information, we are only
> @@ -4147,7 +4184,7 @@ arm_register_type (struct gdbarch *gdbarch, int regnum)
> 
>       if (regnum >= ARM_D0_REGNUM && regnum < ARM_D0_REGNUM + 32
> 	  && t->code () == TYPE_CODE_FLT
> -	  && gdbarch_tdep (gdbarch)->have_neon)
> +	  && tdep->have_neon)
> 	return arm_neon_double_type (gdbarch);
>       else
> 	return t;
> @@ -4155,7 +4192,7 @@ arm_register_type (struct gdbarch *gdbarch, int regnum)
> 
>   if (regnum >= ARM_F0_REGNUM && regnum < ARM_F0_REGNUM + NUM_FREGS)
>     {
> -      if (!gdbarch_tdep (gdbarch)->have_fpa_registers)
> +      if (!tdep->have_fpa_registers)
> 	return builtin_type (gdbarch)->builtin_void;
> 
>       return arm_ext_type (gdbarch);
> @@ -8551,10 +8588,9 @@ show_disassembly_style_sfunc (struct ui_file *file, int from_tty,
> static const char *
> arm_register_name (struct gdbarch *gdbarch, int i)
> {
> -  const int num_regs = gdbarch_num_regs (gdbarch);
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> 
> -  if (gdbarch_tdep (gdbarch)->have_vfp_pseudos
> -      && i >= num_regs && i < num_regs + 32)
> +  if (is_vfp_pseudo (gdbarch, i))
>     {
>       static const char *const vfp_pseudo_names[] = {
> 	"s0", "s1", "s2", "s3", "s4", "s5", "s6", "s7",
> @@ -8563,18 +8599,17 @@ arm_register_name (struct gdbarch *gdbarch, int i)
> 	"s24", "s25", "s26", "s27", "s28", "s29", "s30", "s31",
>       };
> 
> -      return vfp_pseudo_names[i - num_regs];
> +      return vfp_pseudo_names[i - tdep->vfp_pseudo_base];
>     }
> 
> -  if (gdbarch_tdep (gdbarch)->have_neon_pseudos
> -      && i >= num_regs + 32 && i < num_regs + 32 + 16)
> +  if (is_q_pseudo (gdbarch, i))
>     {
> -      static const char *const neon_pseudo_names[] = {
> +      static const char *const q_pseudo_names[] = {
> 	"q0", "q1", "q2", "q3", "q4", "q5", "q6", "q7",
> 	"q8", "q9", "q10", "q11", "q12", "q13", "q14", "q15",
>       };
> 
> -      return neon_pseudo_names[i - num_regs - 32];
> +      return q_pseudo_names[i - tdep->q_pseudo_base];
>     }
> 
>   if (i >= ARRAY_SIZE (arm_register_names))
> @@ -8582,6 +8617,7 @@ arm_register_name (struct gdbarch *gdbarch, int i)
>        an XML description.  */
>     return "";
> 
> +  /* Non-pseudo registers.  */
>   return arm_register_names[i];
> }
> 
> @@ -8719,15 +8755,20 @@ arm_pseudo_read (struct gdbarch *gdbarch, readable_regcache *regcache,
>   int offset, double_regnum;
> 
>   gdb_assert (regnum >= num_regs);
> -  regnum -= num_regs;
> 
> -  if (gdbarch_tdep (gdbarch)->have_neon_pseudos && regnum >= 32 && regnum < 48)
> -    /* Quad-precision register.  */
> -    return arm_neon_quad_read (gdbarch, regcache, regnum - 32, buf);
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  if (is_q_pseudo (gdbarch, regnum))
> +    {
> +      /* Quad-precision register.  */
> +      return arm_neon_quad_read (gdbarch, regcache,
> +				 regnum - tdep->q_pseudo_base, buf);
> +    }
>   else
>     {
>       enum register_status status;
> 
> +      regnum -= tdep->vfp_pseudo_base;
>       /* Single-precision register.  */
>       gdb_assert (regnum < 32);
> 
> @@ -8787,13 +8828,18 @@ arm_pseudo_write (struct gdbarch *gdbarch, struct regcache *regcache,
>   int offset, double_regnum;
> 
>   gdb_assert (regnum >= num_regs);
> -  regnum -= num_regs;
> 
> -  if (gdbarch_tdep (gdbarch)->have_neon_pseudos && regnum >= 32 && regnum < 48)
> -    /* Quad-precision register.  */
> -    arm_neon_quad_write (gdbarch, regcache, regnum - 32, buf);
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  if (is_q_pseudo (gdbarch, regnum))
> +    {
> +      /* Quad-precision register.  */
> +      arm_neon_quad_write (gdbarch, regcache,
> +			   regnum - tdep->q_pseudo_base, buf);
> +    }
>   else
>     {
> +      regnum -= tdep->vfp_pseudo_base;
>       /* Single-precision register.  */
>       gdb_assert (regnum < 32);
> 
> @@ -8940,11 +8986,12 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>   int i;
>   bool is_m = false;
>   int vfp_register_count = 0;
> -  bool have_vfp_pseudos = false, have_neon_pseudos = false;
> +  bool have_vfp_pseudos = false, have_q_pseudos = false;
>   bool have_wmmx_registers = false;
>   bool have_neon = false;
>   bool have_fpa_registers = true;
>   const struct target_desc *tdesc = info.target_desc;
> +  int register_count = ARM_NUM_REGS;
> 
>   /* If we have an object to base this architecture on, try to determine
>      its ABI.  */
> @@ -9248,7 +9295,7 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> 		 their type; otherwise (normally) provide them with
> 		 the default type.  */
> 	      if (tdesc_unnumbered_register (feature, "q0") == 0)
> -		have_neon_pseudos = true;
> +		have_q_pseudos = true;
> 
> 	      have_neon = true;
> 	    }
> @@ -9299,7 +9346,7 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
> 	      || vfp_register_count == 32);
>   tdep->vfp_register_count = vfp_register_count;
>   tdep->have_vfp_pseudos = have_vfp_pseudos;
> -  tdep->have_neon_pseudos = have_neon_pseudos;
> +  tdep->have_q_pseudos = have_q_pseudos;
>   tdep->have_neon = have_neon;
> 
>   arm_register_g_packet_guesses (gdbarch);
> @@ -9387,7 +9434,7 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>   /* Information about registers, etc.  */
>   set_gdbarch_sp_regnum (gdbarch, ARM_SP_REGNUM);
>   set_gdbarch_pc_regnum (gdbarch, ARM_PC_REGNUM);
> -  set_gdbarch_num_regs (gdbarch, ARM_NUM_REGS);
> +  set_gdbarch_num_regs (gdbarch, register_count);
>   set_gdbarch_register_type (gdbarch, arm_register_type);
>   set_gdbarch_register_reggroup_p (gdbarch, arm_register_reggroup_p);
> 
> @@ -9475,21 +9522,29 @@ arm_gdbarch_init (struct gdbarch_info info, struct gdbarch_list *arches)
>       set_tdesc_pseudo_register_name (gdbarch, arm_register_name);
> 
>       tdesc_use_registers (gdbarch, tdesc, std::move (tdesc_data));
> +      register_count = gdbarch_num_regs (gdbarch);
> 
>       /* Override tdesc_register_type to adjust the types of VFP
> 	 registers for NEON.  */
>       set_gdbarch_register_type (gdbarch, arm_register_type);
>     }
> 
> -  if (have_vfp_pseudos)
> +  /* Initialize the pseudo register data.  */
> +  if (tdep->have_vfp_pseudos)
>     {
> -      /* NOTE: These are the only pseudo registers used by
> -	 the ARM target at the moment.  If more are added, a
> -	 little more care in numbering will be needed.  */
> +      /* VFP single precision pseudo registers (S0~S31).  */
> +      tdep->vfp_pseudo_base = register_count;
> +      tdep->vfp_pseudo_count = 32;
> +      int num_pseudos = tdep->vfp_pseudo_count;
> +
> +      if (tdep->have_q_pseudos)
> +	{
> +	  /* NEON quad precision pseudo registers (Q0~Q15).  */
> +	  tdep->q_pseudo_base = register_count + num_pseudos;
> +	  tdep->q_pseudo_count = 16;
> +	  num_pseudos += tdep->q_pseudo_count;
> +	}
> 
> -      int num_pseudos = 32;
> -      if (have_neon_pseudos)
> -	num_pseudos += 16;
>       set_gdbarch_num_pseudo_regs (gdbarch, num_pseudos);
>       set_gdbarch_pseudo_register_read (gdbarch, arm_pseudo_read);
>       set_gdbarch_pseudo_register_write (gdbarch, arm_pseudo_write);
> @@ -9526,10 +9581,18 @@ arm_dump_tdep (struct gdbarch *gdbarch, struct ui_file *file)
> 		      (int) tdep->have_wmmx_registers);
>   fprintf_unfiltered (file, _("arm_dump_tdep: vfp_register_count = %i\n"),
> 		      (int) tdep->vfp_register_count);
> -  fprintf_unfiltered (file, _("arm_dump_tdep: have_vfp_pseudos = %i\n"),
> -		      (int) tdep->have_vfp_pseudos);
> -  fprintf_unfiltered (file, _("arm_dump_tdep: have_neon_pseudos = %i\n"),
> -		      (int) tdep->have_neon_pseudos);
> +  fprintf_unfiltered (file, _("arm_dump_tdep: have_vfp_pseudos = %s\n"),
> +		      tdep->have_vfp_pseudos? "true" : "false");
> +  fprintf_unfiltered (file, _("arm_dump_tdep: vfp_pseudo_base = %i\n"),
> +		      (int) tdep->vfp_pseudo_base);
> +  fprintf_unfiltered (file, _("arm_dump_tdep: vfp_pseudo_count = %i\n"),
> +		      (int) tdep->vfp_pseudo_count);
> +  fprintf_unfiltered (file, _("arm_dump_tdep: have_q_pseudos = %s\n"),
> +		      tdep->have_q_pseudos? "true" : "false");
> +  fprintf_unfiltered (file, _("arm_dump_tdep: q_pseudo_base = %i\n"),
> +		      (int) tdep->q_pseudo_base);
> +  fprintf_unfiltered (file, _("arm_dump_tdep: q_pseudo_count = %i\n"),
> +		      (int) tdep->q_pseudo_count);
>   fprintf_unfiltered (file, _("arm_dump_tdep: have_neon = %i\n"),
> 		      (int) tdep->have_neon);
>   fprintf_unfiltered (file, _("arm_dump_tdep: Lowest pc = 0x%lx\n"),
> diff --git a/gdb/arm-tdep.h b/gdb/arm-tdep.h
> index 969e121b55d..614c1a00ab6 100644
> --- a/gdb/arm-tdep.h
> +++ b/gdb/arm-tdep.h
> @@ -102,9 +102,17 @@ struct gdbarch_tdep
>   int vfp_register_count;
>   bool have_vfp_pseudos;	/* Are we synthesizing the single precision
> 				   VFP registers?  */
> -  bool have_neon_pseudos;	/* Are we synthesizing the quad precision
> -				   NEON registers?  Requires
> +  int vfp_pseudo_base;		/* Register number for the first single
> +				   precision VFP pseudo register.  */
> +  int vfp_pseudo_count;		/* Number of single precision VFP pseudo
> +				   registers.  */
> +  bool have_q_pseudos;		/* Are we synthesizing the quad precision
> +				   Q (NEON or MVE) registers?  Requires
> 				   have_vfp_pseudos.  */
> +  int q_pseudo_base;		/* Register number for the first quad
> +				   precision pseudo register.  */
> +  int q_pseudo_count;		/* Number of quad precision pseudo
> +				   registers.  */
>   bool have_neon;		/* Do we have a NEON unit?  */
> 
>   bool is_m;			/* Does the target follow the "M" profile.  */
> -- 
> 2.25.1
>