Re: [PATCH, ARM 1/7] Consolidate Thumb-1/Thumb-2 ISA detection

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

On 17/12/15 02:14, 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 fixes the code responsible for the Thumb ISA detection. ARM backend of GAS contains the macro ARM_ARCH_THUMB2 and its associated variable arm_ext_t2 to determine the extensions that makes a Thumb code being recognized as Thumb-2. However, these are misused in a number of cases and the definition of ARM_ARCH_THUMB2 include some extension that don't contain any Thumb-2 instruction. This patch cleans all that which paves the way for correct Thumb ISA detection when ARMv8-M Baseline comes into the picture.
> 
> [1] For a quick overview of ARMv8-M please refer to the initial cover letter.
> 
> The reasoning for each existing use of arm_arch_t2 is as follows:
> 
> * move_or_literal_pool: check is for testing availability of specific instructions so use the right extension bit for them
> * handle_it_state: likewise
> * md_assemble: comment is explicit, only architecture with Thumb-2 can use VFP so arm_arch_t2 is correct here
> * md_convert_frag: checking , given a Thumb-2 instruction is present, whether v6t2 extension bit should be set -> no need if any Thumb-2 extension bit is already there so arm_arch_t2 is correct here
> * md_apply_fix: checking for availability of bl/blx with extended range which is a feature of ARMv6t2
> 
> The patch also adds functions wide_insn_ok () to centralize the logic of what instruction can be promoted from 16bit to 32bit encoding and whether Thumb-2 is needed for a given wide encoding. Although these are two separate decisions to make, the logic is the same when ARMv8-M is out of the picture.
> 
> ChangeLog entries are as follow:
> 
> 
> *** gas/ChangeLog ***
> 
> 2015-12-14  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>         * config/tc-arm.c (move_or_literal_pool): Check mov.w, mvm and movw
>         availability against arm_ext_v6t2 instead of checking arm_arch_t2,
>         fixing comments along the way.
>         (handle_it_state): Check arm_ext_v6t2 instead of arm_arch_t2 to
>         generate IT instruction.
>         (non_v6t2_wide_only_insn): New function.
>         (md_assemble): Use above new function to check for invalid wide
>         instruction for CPU Thumb ISA and to determine what Thumb extension
>         bit is necessary for that instruction.
>         (md_apply_fix): Use arm_ext_v6t2 instead of arm_arch_t2 to decide if
>         branch is out of range.
> 
> 
> *** include/opcode/ChangeLog ***
> 
> 2015-12-10  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>         * arm.h (ARM_ARCH_THUMB2): Add comment explaining its meaning and
>         remove extension bit not including any Thumb-2 instruction.
> 

non_v6t2_wide_only_insn should be rewritten to be v6t2_wide_only_insn
with its return values giving the opposite sense.  I really don't like
functions that directly imply a negative in their name as the logic
becomes very confusing when the result is then inverted.  I note that
every single use of this function in the patch uses "!non_...".

Otherwise this is OK.

I'll note for the record here that the use of Thumb-1 and Thumb-2 is now
somewhat anachronistic.  It would be better if the internal variables
and terminology could be cleaned up to talk about T16 and T32 encodings
(I think that's also somewhat cleaner than wide and narrow).  However,
that's a more general cleanup patch and doesn't specifically apply to
the changes here.

R.

> 
> diff --git a/gas/config/tc-arm.c b/gas/config/tc-arm.c
> index 7289a13994b8dce9f3967e0f124c1d1f99894af3..fddb31a9c1a4f654e6006293915a41f1b7d6d7af 100644
> --- a/gas/config/tc-arm.c
> +++ b/gas/config/tc-arm.c
> @@ -7848,10 +7848,10 @@ move_or_literal_pool (int i, enum lit_type t, bfd_boolean mode_3)
>  		  return TRUE;
>  		}
>  
> -	      if (ARM_CPU_HAS_FEATURE (cpu_variant, arm_arch_t2))
> +	      if (ARM_CPU_HAS_FEATURE (cpu_variant, arm_ext_v6t2))
>  		{
> -		  /* Check if on thumb2 it can be done with a mov.w or mvn.w
> -		     instruction.  */
> +		  /* Check if on thumb2 it can be done with a mov.w, mvn or
> +		     movw instruction.  */
>  		  unsigned int newimm;
>  		  bfd_boolean isNegated;
>  
> @@ -7865,19 +7865,22 @@ move_or_literal_pool (int i, enum lit_type t, bfd_boolean mode_3)
>  			isNegated = TRUE;
>  		    }
>  
> +		  /* The number can be loaded with a mov.w or mvn
> +		     instruction.  */
>  		  if (newimm != (unsigned int) FAIL)
>  		    {
> -		      inst.instruction = (0xf04f0000
> +		      inst.instruction = (0xf04f0000  /*  MOV.W.  */
>  					  | (inst.operands[i].reg << 8));
> +		      /* Change to MOVN.  */
>  		      inst.instruction |= (isNegated ? 0x200000 : 0);
>  		      inst.instruction |= (newimm & 0x800) << 15;
>  		      inst.instruction |= (newimm & 0x700) << 4;
>  		      inst.instruction |= (newimm & 0x0ff);
>  		      return TRUE;
>  		    }
> +		  /* The number can be loaded with a movw instruction.  */
>  		  else if ((v & ~0xFFFF) == 0)
>  		    {
> -		      /* The number can be loaded with a mov.w instruction.  */
>  		      int imm = v & 0xFFFF;
>  
>  		      inst.instruction = 0xf2400000;  /* MOVW.  */
> @@ -17536,7 +17539,7 @@ handle_it_state (void)
>  	  else
>  	    {
>  	      if ((implicit_it_mode & IMPLICIT_IT_MODE_THUMB)
> -		  && ARM_CPU_HAS_FEATURE (cpu_variant, arm_arch_t2))
> +		  && ARM_CPU_HAS_FEATURE (cpu_variant, arm_ext_v6t2))
>  		{
>  		  /* Automatically generate the IT instruction.  */
>  		  new_automatic_it_block (inst.cond);
> @@ -17768,6 +17771,22 @@ in_it_block (void)
>    return now_it.state != OUTSIDE_IT_BLOCK;
>  }
>  
> +/* Given an OPCODE that is valid in at least one architecture that is not a
> +   superset of ARMv6t2, returns whether it only has wide encoding(s).  */
> +
> +static bfd_boolean
> +non_v6t2_wide_only_insn (const struct asm_opcode *opcode)
> +{
> +  /* Thumb-1 wide instruction.  */
> +  if (opcode->tencode == do_t_blx
> +      || opcode->tencode == do_t_branch23
> +      || ARM_CPU_HAS_FEATURE (*opcode->tvariant, arm_ext_msr)
> +      || ARM_CPU_HAS_FEATURE (*opcode->tvariant, arm_ext_barrier))
> +    return TRUE;
> +
> +  return FALSE;
> +}
> +
>  void
>  md_assemble (char *str)
>  {
> @@ -17827,24 +17846,24 @@ md_assemble (char *str)
>  	  return;
>  	}
>  
> -      if (!ARM_CPU_HAS_FEATURE (variant, arm_ext_v6t2))
> +      /* Two things are addressed here:
> +	 1) Implicit require narrow instructions on Thumb-1.
> +	    This avoids relaxation accidentally introducing Thumb-2
> +	    instructions.
> +	 2) Reject wide instructions in non Thumb-2 cores.
> +
> +	 Only instructions with narrow and wide variants need to be handled
> +	 but selecting all non wide-only instructions is easier.  */
> +      if (!ARM_CPU_HAS_FEATURE (variant, arm_ext_v6t2)
> +	  && !non_v6t2_wide_only_insn (opcode))
>  	{
> -	  if (opcode->tencode != do_t_blx && opcode->tencode != do_t_branch23
> -	      && !(ARM_CPU_HAS_FEATURE(*opcode->tvariant, arm_ext_msr)
> -		   || ARM_CPU_HAS_FEATURE(*opcode->tvariant, arm_ext_barrier)))
> +	  if (inst.size_req == 0)
> +	    inst.size_req = 2;
> +	  else if (inst.size_req == 4)
>  	    {
> -	      /* Two things are addressed here.
> -		 1) Implicit require narrow instructions on Thumb-1.
> -		    This avoids relaxation accidentally introducing Thumb-2
> -		     instructions.
> -		 2) Reject wide instructions in non Thumb-2 cores.  */
> -	      if (inst.size_req == 0)
> -		inst.size_req = 2;
> -	      else if (inst.size_req == 4)
> -		{
> -		  as_bad (_("selected processor does not support `%s' in Thumb-2 mode"), str);
> -		  return;
> -		}
> +	      as_bad (_("selected processor does not support `%s' in Thumb-2 "
> +			"mode"), str);
> +	      return;
>  	    }
>  	}
>  
> @@ -17879,13 +17898,10 @@ md_assemble (char *str)
>        ARM_MERGE_FEATURE_SETS (thumb_arch_used, thumb_arch_used,
>  			      *opcode->tvariant);
>        /* Many Thumb-2 instructions also have Thumb-1 variants, so explicitly
> -	 set those bits when Thumb-2 32-bit instructions are seen.  ie.
> -	 anything other than bl/blx and v6-M instructions.
> -	 The impact of relaxable instructions will be considered later after we
> -	 finish all relaxation.  */
> -      if ((inst.size == 4 && (inst.instruction & 0xf800e800) != 0xf000e800)
> -	  && !(ARM_CPU_HAS_FEATURE (*opcode->tvariant, arm_ext_msr)
> -	       || ARM_CPU_HAS_FEATURE (*opcode->tvariant, arm_ext_barrier)))
> +	 set those bits when Thumb-2 32-bit instructions are seen.  The impact
> +	 of relaxable instructions will be considered later after we finish all
> +	 relaxation.  */
> +      if (inst.size == 4 && !non_v6t2_wide_only_insn (opcode))
>  	ARM_MERGE_FEATURE_SETS (thumb_arch_used, thumb_arch_used,
>  				arm_ext_v6t2);
>  
> @@ -22853,7 +22869,7 @@ md_apply_fix (fixS *	fixP,
>  
>        if ((value & ~0x3fffff) && ((value & ~0x3fffff) != ~0x3fffff))
>  	{
> -	  if (!(ARM_CPU_HAS_FEATURE (cpu_variant, arm_arch_t2)))
> +	  if (!(ARM_CPU_HAS_FEATURE (cpu_variant, arm_ext_v6t2)))
>  	    as_bad_where (fixP->fx_file, fixP->fx_line, BAD_RANGE);
>  	  else if ((value & ~0x1ffffff)
>  		   && ((value & ~0x1ffffff) != ~0x1ffffff))
> diff --git a/include/opcode/arm.h b/include/opcode/arm.h
> index 286016bbf2a1bc9b3ee58eff9ffae5ccdf602e23..abbe6a82206450aa525b36f3679379d538fe6e1e 100644
> --- a/include/opcode/arm.h
> +++ b/include/opcode/arm.h
> @@ -261,10 +261,10 @@
>  #define ARM_ANY		ARM_FEATURE (-1, -1, 0)	/* Any basic core.  */
>  #define ARM_FEATURE_ALL	ARM_FEATURE (-1, -1, -1)/* All CPU and FPU features.  */
>  #define FPU_ANY_HARD	ARM_FEATURE_COPROC (FPU_FPA | FPU_VFP_HARD | FPU_MAVERICK)
> +/* Extensions containing some Thumb-2 instructions.  If any is present, Thumb
> +   ISA is Thumb-2.  */
>  #define ARM_ARCH_THUMB2 ARM_FEATURE_CORE_LOW (ARM_EXT_V6T2 | ARM_EXT_V7	\
> -					      | ARM_EXT_V7A | ARM_EXT_V7R \
> -					      | ARM_EXT_V7M | ARM_EXT_DIV \
> -					      | ARM_EXT_V8)
> +					      | ARM_EXT_DIV | ARM_EXT_V8)
>  /* v7-a+sec.  */
>  #define ARM_ARCH_V7A_SEC ARM_FEATURE_CORE_LOW (ARM_AEXT_V7A | ARM_EXT_SEC)
>  /* v7-a+mp+sec.  */
> 
> 
> 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
> 
> Is this ok for master branch?
> 
> Best regards,
> 
> Thomas
>