Re: [PATCH-ppc 1/5] Add basic support for new VSX register set

["Ulrich Weigand" <uweigand at de dot ibm dot com>]

Luis Machado wrote:

> 2008-08-08  Luis Machado  <luisgpm@br.ibm.com>
> 
> 	* rs6000-tdep: Include "features/rs6000/powerpc-vsx32.c".
> 	Include "features/rs6000/powerpc-vsx64.c".
> 	(ppc_supply_vsxregset): New function.
> 	(ppc_collect_vsxregset): New function.
> 	(IS_VSX_PSEUDOREG): New macro.
> 	(IS_EFP_PSEUDOREG): New macro.
> 	(vsx_register_p): New function.
> 	(ppc_vsx_support_p): New function.
> 	(rs6000_builtin_type_vec128): New function.
> 	(rs6000_register_name): Hide upper halves of vs0~vs31.  Return
> 	correct names for VSX registers and EFPR registers.
> 	(rs6000_pseudo_register_type): Return correct types for VSX
> 	and EFPR registers.
> 	(rs6000_pseudo_register_reggroup_p): Return correct group for
> 	VSX and EFPR registers.
> 	(ppc_pseudo_register_read): Rename to dfp_pseudo_register_read.
> 	(ppc_pseudo_register_write): Rename to dfp_pseudo_register_write.
> 	(vsx_pseudo_register_read): New function.
> 	(vsx_pseudo_register_write): New function.
> 	(efpr_pseudo_register_read): New function.
> 	(efpr_pseudo_register_write): New function.
> 	(rs6000_pseudo_register_read): Call new VSX and EFPR read functions.
> 	(rs6000_pseudo_register_write): Call new VSX and EFPR write functions.
> 	(rs6000_gdbarch_init): Declare have_vsx.
> 	Initialize new upper half VSX registers.
> 	Initialize VSX-related and EFPR-related pseudo-registers variables.
> 	Adjust the number of pseudo registers accordingly.
> 	
> 	* ppc-linux-nat.c: Define PTRACE_GETVSXREGS, PTRACE_SETVSXREGS
> 	and SIZEOF_VSRREGS.
> 	(gdb_vsxregset_t): New type.
> 	(have_ptrace_getsetvsxregs): New variable.
> 	(fetch_vsx_register): New function.	
> 	(fetch_register): Handle VSX registers.
> 	(fetch_vsx_registers): New function.
> 	(fetch_ppc_registers): Handle VSX registers.
> 	(store_ppc_registers): Handle VSX registers.
> 	(store_vsx_register): New function.	
> 	(store_register): Handle VSX registers.
> 	(store_vsx_registers): New function.
> 	(ppc_linux_read_description): Handle VSX-enabled inferiors.
> 	(gdb_vsxregset_t): New type.
> 	(supply_vsxregset): New function.
> 	(fill_vsxregset): New function.
> 
> 	* ppc-tdep.h (vsx_register_p): New prototype.
> 	(vsx_support_p): New prototype.
> 	(ppc_vsr0_regnum): New variable.
> 	(ppc_vsr0_upper_regnum): Likewise.
> 	(ppc_efpr0_regnum): Likewise.
> 	(ppc_builtin_type_vec128): New type.
> 	(ppc_num_vsrs): New constant.
> 	(ppc_num_efprs): Likewise.
> 	Define POWERPC_VEC_VSX PPC_VSR0_UPPER_REGNUM and PPC_VSR31_UPPER_REGNUM.
> 	(ppc_supply_vsxregset): New prototype.
> 	(ppc_collect_vsxregset): New prototype.
> 
> 	* ppc-linux-tdep.c: Include "features/rs6000/powerpc-vsx32l.c"
> 	Include "features/rs6000/powerpc-vsx64l.c".
> 	(_initialize_ppc_linux_tdep): Initialize VSX-enabled targets.
> 
> 	* ppc-linux-tdep.h: Declare *tdesc_powerpc_vsx32l
> 	Declare tdesc_powerpc_vsx64l



> +/* Supply register REGNUM in the VSX register set REGSET
> +   from the buffer specified by VSRREGS and LEN to register cache
VSXREGS
> +   REGCACHE.  If REGNUM is -1, do this for all registers in REGSET.  */

> +/* Collect register REGNUM in the VSX register set
> +   REGSET from register cache REGCACHE into the buffer specified by
> +   VSRREGS and LEN.  If REGNUM is -1, do this for all registers in
VSXREGS
> +   REGSET.  */

> +      t = init_composite_type ("__ppc_builtin_type_vec128", TYPE_CODE_UNION);
> +      append_composite_type_field (t, "uint128", builtin_type_uint128);
> +      append_composite_type_field (t, "v4_float",
> +				   init_vector_type (builtin_type_float, 4));
> +      append_composite_type_field (t, "v4_int32",
> +				   init_vector_type (builtin_type_int32, 4));
> +      append_composite_type_field (t, "v8_int16",
> +				   init_vector_type (builtin_type_int16, 8));
> +      append_composite_type_field (t, "v16_int8",
> +				   init_vector_type (builtin_type_int8, 16));

Please use builtin_type (gdbarch)->builtin_... instead of builtin_type_...
I'm just trying to get rid of those ...

> @@ -3307,9 +3622,26 @@
>    /* Set the register number for _Decimal128 pseudo-registers.  */
>    tdep->ppc_dl0_regnum = have_dfp? gdbarch_num_regs (gdbarch) : -1;
>  
> +  /* Set the register number for VSX pseudo-registers.  */
> +  tdep->ppc_vsr0_regnum = have_vsx? gdbarch_num_regs (gdbarch) : -1;
> +  tdep->ppc_efpr0_regnum = have_vsx? gdbarch_num_regs (gdbarch) : -1;
> +
>    if (have_dfp && have_spe)
> -    /* Put the _Decimal128 pseudo-registers after the SPE registers.  */
> -    tdep->ppc_dl0_regnum += 32;
> +    {
> +      /* Put the _Decimal128 pseudo-registers after the SPE registers.  */
> +      tdep->ppc_dl0_regnum += 32;
> +
> +      if (have_vsx)
> +	{
> +	  tdep->ppc_vsr0_regnum += 48;
> +	  tdep->ppc_efpr0_regnum += 112;
> +	}
> +    }
> +  else if (have_dfp && have_vsx)
> +    {
> +      tdep->ppc_vsr0_regnum += 16;
> +      tdep->ppc_efpr0_regnum += 80;
> +    }

This logic seems a bit convoluted.   Why not something along those lines:

  /* Choose register numbers for all supported pseudo-registers.  */
  tdep->ppc_ev0_regnum = -1;
  tdep->ppc_dl0_regnum = -1;
  tdep->ppc_vsr0_regnum = -1;
  tdep->ppc_epf0_regnum = -1;

  cur_reg = gdbarch_num_regs (gdbarch);

  if (have_spe)
    {
      tdep->ppc_ev0_regnum = cur_reg;
      cur_reg += 32;
    }
  if (have_dfp)
    {
      tdep->ppc_dl0_regnum = cur_reg;
      cur_reg += 16;
    }
  if (have_vsx)
    {
      tdep->ppc_vsr0_regnum = cur_reg;
      cur_reg += 64;
      tdep->ppc_efpr0_regnum = cur_reg;
      cur_reg += 32;
    }

  gdb_assert (gdbarch_num_regs (gdbarch)
	      + gdbarch_num_pseudo_reg (gdbarch) == cur_reg);


Otherwise, this looks good to me.

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com