Re: [PATCH 5/10, GAS/ARM] Allow Thumb division as an extension for ARMv7

["Richard Earnshaw (lists)" <Richard dot Earnshaw at arm dot com>]

On 21/06/17 11:08, Thomas Preudhomme wrote:
> Hi,
> 
> === Context ===
> 
> This patch is part of a patch series to add support for ARMv8-R
> architecture. Its purpose is to allow ARMv7 to be selected in automatic
> architecture selection in presence of Thumb division instructions.
> 
> === Motivation ===
> 
> any-idiv.d and automatic-sdiv.d testcases in GAS testsuite expect
> autodetection code to select ARMv7 in presence of Thumb integer
> division. However, the definition of ARM_AEXT_V7 and thus ARM_ARCH_V7 do
> not contain these instructions and the idiv extension is only available
> for ARMv7-A and ARMv7-R. Therefore, under the stricter automatic
> detection code proposed in the subsequent patch of the series ARMv7 is
> refused if a Thumb division instruction is present.
> 
> === Patch description ===
> 
> This patch adds a new "idiv" extension after the existing one that is
> available to all ARMv7 targets. This new entry is ignored by all current
> code parsing arm_extensions such that it would be unavailable on the
> command-line and remain a purely internal hack, easily removed in favor
> of a better solution later. This is considered though by the subsequent
> patch reworking automatic detection of build attributes such that ARMv7
> is allowed to match in present of Thumb division instructions. For good
> measure, comments are added in all instances of code browsing
> arm_extensions to mention the expected behavior in case of duplicate
> entries as well as a new testcase.
> 
> ChangeLog entry is as follows:
> 
> *** gas/ChangeLog ***
> 
> 2017-06-13  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>     * config/tc-arm.c (arm_extensions): New duplicate idiv entry to enable
>     Thumb division for ARMv7 architecture.
>     (arm_parse_extension): Document expected behavior for duplicate
>     entries.
>     (s_arm_arch_extension): Likewise.
>     * testsuite/gas/arm/forbid-armv7-idiv-ext.d: New test.
>     * testsuite/gas/arm/forbid-armv7-idiv-ext.l: New expected output for
>     above test.
> 
> === Testing ===
> 
> Testsuite shows no regression when run for arm-none-eabi targets.
> 
> Is this ok for master branch?
> 

It's a bit ugly, but ok.

R.

> Best regards,
> 
> Thomas
> 
> 05_allow_thumb_div_extension_armv7.patch
> 
> 
> diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
> index dfb5779ff6239699454f20403253e92ba36fb4ad..390698ef9272b96f5b4670ecdbd7c0a860658961 100644
> --- a/gas/config/tc-arm.c
> +++ b/gas/config/tc-arm.c
> @@ -25933,6 +25933,13 @@ static const struct arm_option_extension_value_table arm_extensions[] =
>  			ARM_FEATURE_CORE_LOW (ARM_EXT_ADIV | ARM_EXT_DIV),
>  			ARM_FEATURE_CORE_LOW (ARM_EXT_V7A),
>  			ARM_FEATURE_CORE_LOW (ARM_EXT_V7R)),
> +  /* Duplicate entry for the purpose of allowing ARMv7 to match in presence of
> +     Thumb divide instruction.  Due to this having the same name as the
> +     previous entry, this will be ignored when doing command-line parsing and
> +     only considered by build attribute selection code.  */
> +  ARM_EXT_OPT ("idiv",	ARM_FEATURE_CORE_LOW (ARM_EXT_DIV),
> +			ARM_FEATURE_CORE_LOW (ARM_EXT_DIV),
> +			ARM_FEATURE_CORE_LOW (ARM_EXT_V7)),
>    ARM_EXT_OPT ("iwmmxt",ARM_FEATURE_COPROC (ARM_CEXT_IWMMXT),
>  			ARM_FEATURE_COPROC (ARM_CEXT_IWMMXT), ARM_ARCH_NONE),
>    ARM_EXT_OPT ("iwmmxt2", ARM_FEATURE_COPROC (ARM_CEXT_IWMMXT2),
> @@ -26165,6 +26172,10 @@ arm_parse_extension (const char *str, const arm_feature_set *opt_set,
>  	    else
>  	      ARM_CLEAR_FEATURE (**ext_set_p, **ext_set_p, opt->clear_value);
>  
> +	    /* Allowing Thumb division instructions for ARMv7 in autodetection
> +	       rely on this break so that duplicate extensions (extensions
> +	       with the same name as a previous extension in the list) are not
> +	       considered for command-line parsing.  */
>  	    break;
>  	  }
>  
> @@ -26998,6 +27009,10 @@ s_arm_arch_extension (int ignored ATTRIBUTE_UNUSED)
>  	ARM_MERGE_FEATURE_SETS (cpu_variant, selected_cpu, *mfpu_opt);
>  	*input_line_pointer = saved_char;
>  	demand_empty_rest_of_line ();
> +	/* Allowing Thumb division instructions for ARMv7 in autodetection rely
> +	   on this return so that duplicate extensions (extensions with the
> +	   same name as a previous extension in the list) are not considered
> +	   for command-line parsing.  */
>  	return;
>        }
>  
> diff --git a/gas/testsuite/gas/arm/forbid-armv7-idiv-ext.d b/gas/testsuite/gas/arm/forbid-armv7-idiv-ext.d
> new file mode 100644
> index 0000000000000000000000000000000000000000..85c7dc3dec4ec29ee668d34020f9af32a4d3e101
> --- /dev/null
> +++ b/gas/testsuite/gas/arm/forbid-armv7-idiv-ext.d
> @@ -0,0 +1,4 @@
> +# name: Forbidden idiv for ARMv7
> +# source: blank.s
> +# as: -march=armv7+idiv
> +#error-output: forbid-armv7-idiv-ext.l
> diff --git a/gas/testsuite/gas/arm/forbid-armv7-idiv-ext.l b/gas/testsuite/gas/arm/forbid-armv7-idiv-ext.l
> new file mode 100644
> index 0000000000000000000000000000000000000000..76208d2bcbc396ccd1ca9d8e8c48011e56afae3d
> --- /dev/null
> +++ b/gas/testsuite/gas/arm/forbid-armv7-idiv-ext.l
> @@ -0,0 +1,3 @@
> +Assembler messages:
> +[^:]*: extension does not apply to the base architecture
> +[^:]*: unrecognized option -march=armv7\+idiv
>