Re: [PATCH, ARM 6/7, ping] Allow extension availability to depend on several architecture bits

[Thomas Preudhomme <thomas dot preudhomme at foss dot arm dot com>]

Ping?

On Thursday 17 December 2015 10:55:06 Thomas Preud'homme wrote:
> Hi,
> 
> This patch is part of a patch series to add support for ARMv8-M[1] to
> binutils. This specific patch refactors extension parsing code to allow the
> addition of the DSP extension to ARMv8-M mainline in the next patch of the
> series. Due to ARMv8-M coming in two profiles (Baseline and Mainline), it
> is necessary to guard the availability of this extension upon the presence
> of the bits ARM_EXT_V8 and ARM_EXT2_V8M since depending only on the
> presence of ARM_AEXT2_V8M would allow the extension for ARMv8-M Baseline as
> well.
> 
> However, currently the extension parsing code interprets several bits being
> set in the allowed_arch as meaning that the extension should be allowed if
> any of the bits is set in the selected architecture. This patch changes the
> meaning of several bits being set and extends allowed_arch into an array to
> cater to the existing need of an extension being allowed for two different
> architectures.
> 
> [1] For a quick overview of ARMv8-M please refer to the initial cover
> letter.
> 
> 
> ChangeLog entry is as follows:
> 
> *** gas/ChangeLog ***
> 
> 2015-11-17  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>         * config/tc-arm.c
>         (struct arm_option_extension_value_table): Make allowed_archs an
> array with 2 entries.
>         (ARM_EXT_OPT): Adapt to only fill the first entry of allowed_archs.
>         (ARM_EXT_OPT2): New macro filling the two entries of allowed_archs.
>         (arm_extensions): Use separate entries in allowed_archs when several
> archs are allowed to use an extension and change ARCH_ANY in ARM_ARCH_NONE
> in allowed_archs.
>         (arm_parse_extension): Check that, for each allowed_archs entry, all
> bits are set in the current architecture, ignoring ARM_ANY entries.
> (s_arm_arch_extension): Likewise.
> 
> 
> *** include/opcode/ChangeLog ***
> 
> 2015-12-15  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>         * arm.h (ARM_CPU_HAS_FEATURE): Add comment.
>         (ARM_FSET_CPU_SUBSET): Define macro.
> 
> 
> Tests done:
> 
> * No regression under binutils testsuite
> * Toolchain was built successfully with and without the ARMv8-M support
> patches[2] with the following multilib list:
> armv6-m,armv7-m,armv7e-m,cortex-m7,armv8-m.base,armv8-m.main. The code
> generation for crtbegin.o, crtend.o, crti.o, crtn.o, libgcc.a, libgcov.a,
> libc.a, libg.a, libgloss-linux.a, libm.a, libnosys.a, librdimon.a,
> librdpmon.a, libstdc++.a and libsupc++.a is unchanged for all targets
> supported before the patches. * Thumb-1 (default arch and
> --with-mode=thumb) and Thumb-2 (--with-arch=armv7-a --with-mode=thumb) GCC
> bootstrap using binutils with this patch * No GCC testsuite regression on
> fast model compared to ARMv6s-M (Baseline) or ARMv7-M (Mainline)
> 
> [2] including this one, the ld one and the GCC one
> 
> diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
> index
> 76357b93fde7df854d39468df1d74cfc0a22456c..007179cf34d68b7b096ade16805f80676
> 3abfcfa 100644 --- a/gas/config/tc-arm.c
> +++ b/gas/config/tc-arm.c
> @@ -24950,12 +24950,16 @@ struct arm_option_extension_value_table
>    size_t name_len;
>    const arm_feature_set merge_value;
>    const arm_feature_set clear_value;
> -  const arm_feature_set allowed_archs;
> +  /* List of architectures for which an extension is available. 
> ARM_ARCH_NONE +     indicates that an extension is available for all
> architectures while +     ARM_ANY marks an empty entry.  */
> +  const arm_feature_set allowed_archs[2];
>  };
> 
>  /* The following table must be in alphabetical order with a NULL last
> entry. */
> -#define ARM_EXT_OPT(N, M, C, AA) { N, sizeof (N) - 1, M, C, AA }
> +#define ARM_EXT_OPT(N, M, C, AA) { N, sizeof (N) - 1, M, C, { AA, ARM_ANY }
> } +#define ARM_EXT_OPT2(N, M, C, AA1, AA2) { N, sizeof (N) - 1, M, C, {AA1,
> AA2} } static const struct arm_option_extension_value_table
> arm_extensions[] = {
>    ARM_EXT_OPT ("crc",  ARCH_CRC_ARMV8, ARM_FEATURE_COPROC (CRC_EXT_ARMV8),
> @@ -24965,18 +24969,20 @@ static const struct
> arm_option_extension_value_table arm_extensions[] = ARM_FEATURE_CORE_LOW
> (ARM_EXT_V8)),
>    ARM_EXT_OPT ("fp",     FPU_ARCH_VFP_ARMV8, ARM_FEATURE_COPROC
> (FPU_VFP_ARMV8), ARM_FEATURE_CORE_LOW (ARM_EXT_V8)),
> -  ARM_EXT_OPT ("idiv",	ARM_FEATURE_CORE_LOW (ARM_EXT_ADIV | ARM_EXT_DIV),
> +  ARM_EXT_OPT2 ("idiv",	ARM_FEATURE_CORE_LOW (ARM_EXT_ADIV | 
ARM_EXT_DIV),
>  			ARM_FEATURE_CORE_LOW (ARM_EXT_ADIV | ARM_EXT_DIV),
> -				   ARM_FEATURE_CORE_LOW (ARM_EXT_V7A | ARM_EXT_V7R)),
> +			ARM_FEATURE_CORE_LOW (ARM_EXT_V7A),
> +			ARM_FEATURE_CORE_LOW (ARM_EXT_V7R)),
>    ARM_EXT_OPT ("iwmmxt",ARM_FEATURE_COPROC (ARM_CEXT_IWMMXT),
> -			ARM_FEATURE_COPROC (ARM_CEXT_IWMMXT), ARM_ANY),
> +			ARM_FEATURE_COPROC (ARM_CEXT_IWMMXT), ARM_ARCH_NONE),
>    ARM_EXT_OPT ("iwmmxt2", ARM_FEATURE_COPROC (ARM_CEXT_IWMMXT2),
> -			ARM_FEATURE_COPROC (ARM_CEXT_IWMMXT2), ARM_ANY),
> +			ARM_FEATURE_COPROC (ARM_CEXT_IWMMXT2), ARM_ARCH_NONE),
>    ARM_EXT_OPT ("maverick", ARM_FEATURE_COPROC (ARM_CEXT_MAVERICK),
> -			ARM_FEATURE_COPROC (ARM_CEXT_MAVERICK), ARM_ANY),
> -  ARM_EXT_OPT ("mp",	ARM_FEATURE_CORE_LOW (ARM_EXT_MP),
> +			ARM_FEATURE_COPROC (ARM_CEXT_MAVERICK), ARM_ARCH_NONE),
> +  ARM_EXT_OPT2 ("mp",	ARM_FEATURE_CORE_LOW (ARM_EXT_MP),
>  			ARM_FEATURE_CORE_LOW (ARM_EXT_MP),
> -				   ARM_FEATURE_CORE_LOW (ARM_EXT_V7A | ARM_EXT_V7R)),
> +			ARM_FEATURE_CORE_LOW (ARM_EXT_V7A),
> +			ARM_FEATURE_CORE_LOW (ARM_EXT_V7R)),
>    ARM_EXT_OPT ("simd",   FPU_ARCH_NEON_VFP_ARMV8,
>  			ARM_FEATURE_COPROC (FPU_NEON_ARMV8),
>  				   ARM_FEATURE_CORE_LOW (ARM_EXT_V8)),
> @@ -24986,9 +24992,10 @@ static const struct
> arm_option_extension_value_table arm_extensions[] = ARM_EXT_OPT
> ("pan",	ARM_FEATURE_CORE_HIGH (ARM_EXT2_PAN),
>  			ARM_FEATURE (ARM_EXT_V8, ARM_EXT2_PAN, 0),
>  			ARM_FEATURE_CORE_LOW (ARM_EXT_V8)),
> -  ARM_EXT_OPT ("sec",	ARM_FEATURE_CORE_LOW (ARM_EXT_SEC),
> +  ARM_EXT_OPT2 ("sec",	ARM_FEATURE_CORE_LOW (ARM_EXT_SEC),
>  			ARM_FEATURE_CORE_LOW (ARM_EXT_SEC),
> -				   ARM_FEATURE_CORE_LOW (ARM_EXT_V6K | ARM_EXT_V7A)),
> +			ARM_FEATURE_CORE_LOW (ARM_EXT_V6K),
> +			ARM_FEATURE_CORE_LOW (ARM_EXT_V7A)),
>    ARM_EXT_OPT ("virt",	ARM_FEATURE_CORE_LOW (ARM_EXT_VIRT | ARM_EXT_ADIV
> 
>  				     | ARM_EXT_DIV),
> 
>  			ARM_FEATURE_CORE_LOW (ARM_EXT_VIRT),
> @@ -24997,8 +25004,8 @@ static const struct arm_option_extension_value_table
> arm_extensions[] = ARM_FEATURE_COPROC (FPU_NEON_ARMV8 | FPU_NEON_EXT_RDMA),
>  				   ARM_FEATURE_CORE_LOW (ARM_EXT_V8)),
>    ARM_EXT_OPT ("xscale",ARM_FEATURE_COPROC (ARM_CEXT_XSCALE),
> -			ARM_FEATURE_COPROC (ARM_CEXT_XSCALE), ARM_ANY),
> -  { NULL, 0, ARM_ARCH_NONE, ARM_ARCH_NONE, ARM_ARCH_NONE }
> +			ARM_FEATURE_COPROC (ARM_CEXT_XSCALE), ARM_ARCH_NONE),
> +  { NULL, 0, ARM_ARCH_NONE, ARM_ARCH_NONE, { ARM_ARCH_NONE, ARM_ARCH_NONE }
> } };
>  #undef ARM_EXT_OPT
> 
> @@ -25105,6 +25112,7 @@ arm_parse_extension (char *str, const
> arm_feature_set **opt_p) or removing it (0) and only allowing it to change
> in the order -1 -> 1 -> 0.  */
>    const struct arm_option_extension_value_table * opt = NULL;
> +  const arm_feature_set arm_any = ARM_ANY;
>    int adding_value = -1;
> 
>    /* Copy the feature set, so that we can modify it.  */
> @@ -25169,8 +25177,18 @@ arm_parse_extension (char *str, const
> arm_feature_set **opt_p) for (; opt->name != NULL; opt++)
>  	if (opt->name_len == len && strncmp (opt->name, str, len) == 0)
>  	  {
> +	    int i, nb_allowed_archs =
> +	      sizeof (opt->allowed_archs) / sizeof (opt->allowed_archs[0]);
>  	    /* Check we can apply the extension to this architecture.  */
> -	    if (!ARM_CPU_HAS_FEATURE (*ext_set, opt->allowed_archs))
> +	    for (i = 0; i < nb_allowed_archs; i++)
> +	      {
> +		/* Empty entry.  */
> +		if (ARM_FEATURE_EQUAL (opt->allowed_archs[i], arm_any))
> +		  continue;
> +		if (ARM_FSET_CPU_SUBSET (opt->allowed_archs[i], *ext_set))
> +		  break;
> +	      }
> +	    if (i == nb_allowed_archs)
>  	      {
>  		as_bad (_("extension does not apply to the base architecture"));
>  		return FALSE;
> @@ -25923,6 +25941,7 @@ static void
>  s_arm_arch_extension (int ignored ATTRIBUTE_UNUSED)
>  {
>    const struct arm_option_extension_value_table *opt;
> +  const arm_feature_set arm_any = ARM_ANY;
>    char saved_char;
>    char *name;
>    int adding_value = 1;
> @@ -25943,7 +25962,18 @@ s_arm_arch_extension (int ignored ATTRIBUTE_UNUSED)
> for (opt = arm_extensions; opt->name != NULL; opt++)
>      if (streq (opt->name, name))
>        {
> -	if (!ARM_CPU_HAS_FEATURE (*mcpu_cpu_opt, opt->allowed_archs))
> +	int i, nb_allowed_archs =
> +	  sizeof (opt->allowed_archs) / sizeof (opt->allowed_archs[i]);
> +	for (i = 0; i < nb_allowed_archs; i++)
> +	  {
> +	    /* Empty entry.  */
> +	    if (ARM_FEATURE_EQUAL (opt->allowed_archs[i], arm_any))
> +	      continue;
> +	    if (ARM_FSET_CPU_SUBSET (opt->allowed_archs[i], *mcpu_cpu_opt))
> +	      break;
> +	  }
> +
> +	if (i == nb_allowed_archs)
>  	  {
>  	    as_bad (_("architectural extension `%s' is not allowed for the "
>  		      "current base architecture"), name);
> diff --git a/include/opcode/arm.h b/include/opcode/arm.h
> index
> 0209f037ad8047993dc2a9bfc88536c3a66c59b2..28d72b46c157f45056883122794b0ef07
> e696454 100644 --- a/include/opcode/arm.h
> +++ b/include/opcode/arm.h
> @@ -319,11 +319,18 @@ typedef struct
>    unsigned long coproc;
>  } arm_feature_set;
> 
> +/* Returns whether one of the feature bits set in FEAT is also set in CPU. 
> */ #define ARM_CPU_HAS_FEATURE(CPU,FEAT) \
>    (((CPU).core[0] & (FEAT).core[0]) != 0 \
> 
>     || ((CPU).core[1] & (FEAT).core[1]) != 0 \
>     || ((CPU).coproc & (FEAT).coproc) != 0)
> 
> +/* Tests whether the features of A are a subset of B.  */
> +#define ARM_FSET_CPU_SUBSET(A,B) \
> +  (((A).core[0] & (B).core[0]) == (A).core[0] \
> +   && ((A).core[1] & (B).core[1]) == (A).core[1] \
> +   && ((A).coproc & (B).coproc) == (A).coproc)
> +
>  #define ARM_CPU_IS_ANY(CPU) \
>    ((CPU).core[0] == ((arm_feature_set)ARM_ANY).core[0] \
>     && (CPU).core[1] == ((arm_feature_set)ARM_ANY).core[1])
> 
> 
> Is this ok for the master branch?
> 
> Best regards,
> 
> Thomas