Hi Kyrill,

On Friday 20 May 2016 14:22:48 Kyrill Tkachov wrote:
> Hi Thomas,
> 
> On 17/05/16 11:14, Thomas Preudhomme wrote:
> > Ping?
> > 
> > *** gcc/ChangeLog ***
> > 
> > 2015-11-13  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> > 
> >          * config/arm/arm.c (arm_print_operand_punct_valid_p): Make %?
> >          valid
> >          for Thumb-1.
> >          * config/arm/arm.h (TARGET_HAVE_CBZ): Define.
> >          (TARGET_IDIV): Set for all Thumb targets provided they have
> >          hardware
> >          divide feature.
> >          * config/arm/thumb1.md (thumb1_cbz): New insn.
> > 
> > diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
> > index
> > f42e996e5a7ce979fe406b8261d50fb2ba005f6b..347b5b0a5cc0bc1e3b5020c8124d968e
> > 76ce48a4 100644
> > --- a/gcc/config/arm/arm.h
> > +++ b/gcc/config/arm/arm.h
> > @@ -271,9 +271,12 @@ extern void (*arm_lang_output_object_attributes_hook)
> > (void);
> > 
> >   /* Nonzero if this chip provides the movw and movt instructions.  */
> >   #define TARGET_HAVE_MOVT	(arm_arch_thumb2 || arm_arch8)
> > 
> > +/* Nonzero if this chip provides the cb{n}z instruction.  */
> > +#define TARGET_HAVE_CBZ		(arm_arch_thumb2 || arm_arch8)
> > +
> > 
> >   /* Nonzero if integer division instructions supported.  */
> >   #define TARGET_IDIV	((TARGET_ARM && arm_arch_arm_hwdiv)	\
> > 
> > -			 || (TARGET_THUMB2 && arm_arch_thumb_hwdiv))
> > +			 || (TARGET_THUMB && arm_arch_thumb_hwdiv))
> > 
> >   /* Nonzero if disallow volatile memory access in IT block.  */
> >   #define TARGET_NO_VOLATILE_CE		(arm_arch_no_volatile_ce)
> > 
> > diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> > index
> > 13b4b71ac8f9c1da8ef1945f7ff6985ca59f6832..445972ce0e3fd27d4411840ff69e9edb
> > b23994fc 100644
> > --- a/gcc/config/arm/arm.c
> > +++ b/gcc/config/arm/arm.c
> > @@ -22684,7 +22684,7 @@ arm_print_operand_punct_valid_p (unsigned char
> > code)> 
> >   {
> >   
> >     return (code == '@' || code == '|' || code == '.'
> >     
> >   	  || code == '(' || code == ')' || code == '#'
> > 
> > -	  || (TARGET_32BIT && (code == '?'))
> > +	  || code == '?'
> > 
> >   	  || (TARGET_THUMB2 && (code == '!'))
> >   	  || (TARGET_THUMB && (code == '_')));
> >   
> >   }
> 
> Hmm, I'm not a fan of this change. arm_print_operand_punct_valid_p is an
> implementation of a target hook that is used to validate user-provided
> inline asm as well and is therefore the right place to reject such invalid
> constructs.
> 
> This is just working around the fact that the output template for the
> [u]divsi3 patterns has a '%?' in it that is illegal in Thumb1 and will not
> be used for ARMv8-M Baseline anyway. I'd prefer it if you add a second
> alternative to those patterns and emit the sdiv/udiv mnemonic without the
> '%?' and enable that for the v8mb arch attribute (and mark the existing
> alternative as requiring the "32" arch attribute).
> > diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md
> > index
> > 4572456b8bc98503061846cad94bc642943db3a2..1b01ef6ce731fe3ff37c3d8c048fb9d5
> > e7829b35 100644
> > --- a/gcc/config/arm/thumb1.md
> > +++ b/gcc/config/arm/thumb1.md
> > @@ -973,6 +973,92 @@
> > 
> >     DONE;
> >   
> >   })
> > 
> > +;; A pattern for the cb(n)z instruction added in ARMv8-M baseline
> > profile,
> > +;; adapted from cbranchsi4_insn.  Modifying cbranchsi4_insn instead leads
> > to +;; code generation difference for ARMv6-M because the minimum length
> > of the +;; instruction becomes 2 even for it due to a limitation in
> > genattrtab's +;; handling of pc in the length condition.
> > +(define_insn "thumb1_cbz"
> > +  [(set (pc) (if_then_else
> > +	      (match_operator 0 "equality_operator"
> > +	       [(match_operand:SI 1 "s_register_operand" "l")
> > +		(const_int 0)])
> > +	      (label_ref (match_operand 2 "" ""))
> > +	      (pc)))]
> > +  "TARGET_THUMB1 && TARGET_HAVE_MOVT"
> > +{
> 
> s/TARGET_HAVE_MOVT/TARGET_HAVE_CBZ/
> 
> > +  if (get_attr_length (insn) == 2)
> > +    {
> > +      if (GET_CODE (operands[0]) == EQ)
> > +	return "cbz\t%1, %l2";
> > +      else
> > +	return "cbnz\t%1, %l2";
> > +    }
> > +  else
> > +    {
> > +      rtx t = cfun->machine->thumb1_cc_insn;
> > +      if (t != NULL_RTX)
> > +	{
> > +	  if (!rtx_equal_p (cfun->machine->thumb1_cc_op0, operands[1])
> > +	      || !rtx_equal_p (cfun->machine->thumb1_cc_op1, operands[2]))
> > +	    t = NULL_RTX;
> > +	  if (cfun->machine->thumb1_cc_mode == CC_NOOVmode)
> > +	    {
> > +	      if (!noov_comparison_operator (operands[0], VOIDmode))
> > +		t = NULL_RTX;
> > +	    }
> > +	  else if (cfun->machine->thumb1_cc_mode != CCmode)
> > +	    t = NULL_RTX;
> > +	}
> > +      if (t == NULL_RTX)
> > +	{
> > +	  output_asm_insn ("cmp\t%1, #0", operands);
> > +	  cfun->machine->thumb1_cc_insn = insn;
> > +	  cfun->machine->thumb1_cc_op0 = operands[1];
> > +	  cfun->machine->thumb1_cc_op1 = operands[2];
> > +	  cfun->machine->thumb1_cc_mode = CCmode;
> > +	}
> > +      else
> > +	/* Ensure we emit the right type of condition code on the jump.  */
> > +	XEXP (operands[0], 0) = gen_rtx_REG (cfun->machine->thumb1_cc_mode,
> > +					     CC_REGNUM);
> > +
> > +      switch (get_attr_length (insn))
> > +	{
> > +	case 4:  return "b%d0\t%l2";
> > +	case 6:  return "b%D0\t.LCB%=;b\t%l2\t%@long jump\n.LCB%=:";
> > +	case 8: return "b%D0\t.LCB%=;bl\t%l2\t%@far jump\n.LCB%=:";
> > +	default: gcc_unreachable ();
> > +	}
> > +    }
> > +}
> > +  [(set (attr "far_jump")
> > +	(if_then_else
> > +	    (eq_attr "length" "8")
> > +	    (const_string "yes")
> > +	    (const_string "no")))
> > +   (set (attr "length")
> > +	(if_then_else
> > +	    (and (ge (minus (match_dup 2) (pc)) (const_int 2))
> > +		 (le (minus (match_dup 2) (pc)) (const_int 128))
> > +		 (not (match_test "which_alternative")))
> 
> This pattern only has one alternative so "which_alternative"
> will always be 0, so the (not (match_test "which_alternative"))
> test inside the 'and' is redundant and can be removed.

What about the attached updated patch?

ChangeLog entry are as follow:


*** gcc/ChangeLog ***

2016-05-23  Thomas Preud'homme  <thomas.preudhomme@arm.com>

        * config/arm/arm.h (TARGET_HAVE_CBZ): Define.
        (TARGET_IDIV): Set for all Thumb targets provided they have hardware
        divide feature.
        * config/arm/arm.md (divsi3): New unpredicable alternative for ARMv8-M
        Baseline.  Make initial alternative TARGET_32BIT only.
        (udivsi3): Likewise.
        * config/arm/thumb1.md (thumb1_cbz): New insn.
        * doc/sourcebuild.texi (arm_thumb1_cbz_ok): Document new effective
        target.


*** gcc/testsuite/ChangeLog ***

2016-05-23  Thomas Preud'homme  <thomas.preudhomme@arm.com>

        * lib/target-supports.exp (check_effective_target_arm_thumb1_cbz_ok):
        Add new arm_thumb1_cbz_ok effective target.
        * gcc.target/arm/cbz.c: New test.


Best regards,

Thomas

> 
> Thanks,
> Kyrill
> 
> > +	    (const_int 2)
> > +	    (if_then_else
> > +		(and (ge (minus (match_dup 2) (pc)) (const_int -250))
> > +		     (le (minus (match_dup 2) (pc)) (const_int 256)))
> > +		(const_int 4)
> > +		(if_then_else
> > +		    (and (ge (minus (match_dup 2) (pc)) (const_int -2040))
> > +			 (le (minus (match_dup 2) (pc)) (const_int 2048)))
> > +		    (const_int 6)
> > +		    (const_int 8)))))
> > +   (set (attr "type")
> > +	(if_then_else
> > +	    (eq_attr "length" "2")
> > +	    (const_string "branch")
> > +	    (const_string "multiple")))]
> > +)
> > +
> > 
> >   (define_insn "cbranchsi4_insn"
> >   
> >     [(set (pc) (if_then_else
> >     
> >   	      (match_operator 0 "arm_comparison_operator"
> > 
> > Best regards,
> > 
> > Thomas
> > 
> > On Thursday 17 December 2015 16:17:52 Thomas Preud'homme wrote:
> >> Hi,
> >> 
> >> This patch is part of a patch series to add support for ARMv8-M[1] to
> >> GCC.
> >> This specific patch makes the compiler start generating code with the new
> >> CB(N)Z and (U|S)DIV instructions for ARMv8-M Baseline.
> >> 
> >> Sharing of instruction patterns for div insn template with ARM or Thumb-2
> >> was done by allowing %? punctuation character for Thumb-1. This is safe
> >> to
> >> do since the compiler would fault in arm_print_condition if a condition
> >> code is not handled by a branch in Thumb1.
> >> 
> >> Unfortunately, cbz cannot be shared with cbranchsi4 because it would lead
> >> to worse code for Thumb-1. Indeed, choosing cb(n)z over the other
> >> alternatives for cbranchsi4 depends on the distance between target and
> >> pc which lead insn-attrtab to evaluate the minimum length of this
> >> pattern to be 2 as it cannot computer the distance statically. It would
> >> be possible to determine that this alternative is not available for non
> >> ARMv8-M Thumb-1 target statically but genattrtab is not currently
> >> capable to do it, so this is for a later patch.
> >> 
> >> 
> >> [1] For a quick overview of ARMv8-M please refer to the initial cover
> >> letter.
> >> 
> >> ChangeLog entry is as follows:
> >> 
> >> 
> >> *** gcc/ChangeLog ***
> >> 
> >> 2015-11-13  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> >> 
> >>          * config/arm/arm.c (arm_print_operand_punct_valid_p): Make %?
> >>          valid
> >>          for Thumb-1.
> >>          * config/arm/arm.h (TARGET_HAVE_CBZ): Define.
> >>          (TARGET_IDIV): Set for all Thumb targets provided they have
> >>          hardware
> >> 
> >> divide feature.
> >> 
> >>          * config/arm/thumb1.md (thumb1_cbz): New insn.
> >> 
> >> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
> >> index 015df50..247f144 100644
> >> --- a/gcc/config/arm/arm.h
> >> +++ b/gcc/config/arm/arm.h
> >> @@ -263,9 +263,12 @@ extern void
> >> (*arm_lang_output_object_attributes_hook)(void); /* Nonzero if this chip
> >> provides the movw and movt instructions.  */ #define
> >> TARGET_HAVE_MOVT	(arm_arch_thumb2 || arm_arch8)
> >> 
> >> +/* Nonzero if this chip provides the cb{n}z instruction.  */
> >> +#define TARGET_HAVE_CBZ		(arm_arch_thumb2 || arm_arch8)
> >> +
> >> 
> >>   /* Nonzero if integer division instructions supported.  */
> >>   #define TARGET_IDIV	((TARGET_ARM && arm_arch_arm_hwdiv)	\
> >> 
> >> -			 || (TARGET_THUMB2 && arm_arch_thumb_hwdiv))
> >> +			 || (TARGET_THUMB && arm_arch_thumb_hwdiv))
> >> 
> >>   /* Nonzero if disallow volatile memory access in IT block.  */
> >>   #define TARGET_NO_VOLATILE_CE		(arm_arch_no_volatile_ce)
> >> 
> >> diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
> >> index d832309..5ef3a1d 100644
> >> --- a/gcc/config/arm/arm.c
> >> +++ b/gcc/config/arm/arm.c
> >> @@ -22568,7 +22568,7 @@ arm_print_operand_punct_valid_p (unsigned char
> >> code) {
> >> 
> >>     return (code == '@' || code == '|' || code == '.'
> >>     
> >>   	  || code == '(' || code == ')' || code == '#'
> >> 
> >> -	  || (TARGET_32BIT && (code == '?'))
> >> +	  || code == '?'
> >> 
> >>   	  || (TARGET_THUMB2 && (code == '!'))
> >>   	  || (TARGET_THUMB && (code == '_')));
> >>   
> >>   }
> >> 
> >> diff --git a/gcc/config/arm/thumb1.md b/gcc/config/arm/thumb1.md
> >> index 7e3bcb4..074b267 100644
> >> --- a/gcc/config/arm/thumb1.md
> >> +++ b/gcc/config/arm/thumb1.md
> >> @@ -973,6 +973,92 @@
> >> 
> >>     DONE;
> >>   
> >>   })
> >> 
> >> +;; A pattern for the cb(n)z instruction added in ARMv8-M baseline
> >> profile,
> >> +;; adapted from cbranchsi4_insn.  Modifying cbranchsi4_insn instead
> >> leads
> >> to +;; code generation difference for ARMv6-M because the minimum length
> >> of
> >> the +;; instruction becomes 2 even for it due to a limitation in
> >> genattrtab's +;; handling of pc in the length condition.
> >> +(define_insn "thumb1_cbz"
> >> +  [(set (pc) (if_then_else
> >> +	      (match_operator 0 "equality_operator"
> >> +	       [(match_operand:SI 1 "s_register_operand" "l")
> >> +		(const_int 0)])
> >> +	      (label_ref (match_operand 2 "" ""))
> >> +	      (pc)))]
> >> +  "TARGET_THUMB1 && TARGET_HAVE_MOVT"
> >> +{
> >> +  if (get_attr_length (insn) == 2)
> >> +    {
> >> +      if (GET_CODE (operands[0]) == EQ)
> >> +	return "cbz\t%1, %l2";
> >> +      else
> >> +	return "cbnz\t%1, %l2";
> >> +    }
> >> +  else
> >> +    {
> >> +      rtx t = cfun->machine->thumb1_cc_insn;
> >> +      if (t != NULL_RTX)
> >> +	{
> >> +	  if (!rtx_equal_p (cfun->machine->thumb1_cc_op0, operands[1])
> >> +	      || !rtx_equal_p (cfun->machine->thumb1_cc_op1, operands[2]))
> >> +	    t = NULL_RTX;
> >> +	  if (cfun->machine->thumb1_cc_mode == CC_NOOVmode)
> >> +	    {
> >> +	      if (!noov_comparison_operator (operands[0], VOIDmode))
> >> +		t = NULL_RTX;
> >> +	    }
> >> +	  else if (cfun->machine->thumb1_cc_mode != CCmode)
> >> +	    t = NULL_RTX;
> >> +	}
> >> +      if (t == NULL_RTX)
> >> +	{
> >> +	  output_asm_insn ("cmp\t%1, #0", operands);
> >> +	  cfun->machine->thumb1_cc_insn = insn;
> >> +	  cfun->machine->thumb1_cc_op0 = operands[1];
> >> +	  cfun->machine->thumb1_cc_op1 = operands[2];
> >> +	  cfun->machine->thumb1_cc_mode = CCmode;
> >> +	}
> >> +      else
> >> +	/* Ensure we emit the right type of condition code on the jump.  */
> >> +	XEXP (operands[0], 0) = gen_rtx_REG (cfun->machine->thumb1_cc_mode,
> >> +					     CC_REGNUM);
> >> +
> >> +      switch (get_attr_length (insn))
> >> +	{
> >> +	case 4:  return "b%d0\t%l2";
> >> +	case 6:  return "b%D0\t.LCB%=;b\t%l2\t%@long jump\n.LCB%=:";
> >> +	case 8: return "b%D0\t.LCB%=;bl\t%l2\t%@far jump\n.LCB%=:";
> >> +	default: gcc_unreachable ();
> >> +	}
> >> +    }
> >> +}
> >> +  [(set (attr "far_jump")
> >> +	(if_then_else
> >> +	    (eq_attr "length" "8")
> >> +	    (const_string "yes")
> >> +	    (const_string "no")))
> >> +   (set (attr "length")
> >> +	(if_then_else
> >> +	    (and (ge (minus (match_dup 2) (pc)) (const_int 2))
> >> +		 (le (minus (match_dup 2) (pc)) (const_int 128))
> >> +		 (not (match_test "which_alternative")))
> >> +	    (const_int 2)
> >> +	    (if_then_else
> >> +		(and (ge (minus (match_dup 2) (pc)) (const_int -250))
> >> +		     (le (minus (match_dup 2) (pc)) (const_int 256)))
> >> +		(const_int 4)
> >> +		(if_then_else
> >> +		    (and (ge (minus (match_dup 2) (pc)) (const_int -2040))
> >> +			 (le (minus (match_dup 2) (pc)) (const_int 2048)))
> >> +		    (const_int 6)
> >> +		    (const_int 8)))))
> >> +   (set (attr "type")
> >> +	(if_then_else
> >> +	    (eq_attr "length" "2")
> >> +	    (const_string "branch")
> >> +	    (const_string "multiple")))]
> >> +)
> >> +
> >> 
> >>   (define_insn "cbranchsi4_insn"
> >>   
> >>     [(set (pc) (if_then_else
> >>     
> >>   	      (match_operator 0 "arm_comparison_operator"
> >> 
> >> Testing:
> >> 
> >> * Toolchain was built successfully with and without the ARMv8-M support
> >> patches with the following multilib list:
> >> armv6-m,armv7-m,armv7e-m,cortex-m7. 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 these targets.
> >> 
> >> * GCC also showed no testsuite regression when targeting ARMv8-M Baseline
> >> compared to ARMv6-M on ARM Fast Models and when targeting ARMv6-M and
> >> ARMv7-M (compared to without the patch) * GCC was bootstrapped
> >> successfully
> >> targeting Thumb-1 and targeting Thumb-2
> >> 
> >> Is this ok for stage3?
> >> 
> >> Best regards,
> >> 
> >> Thomas