This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, ARM 6/7, ping1] Add support for CB(N)Z and (U|S)DIV to ARMv8-M Baseline



On 12/07/16 11:26, Thomas Preudhomme wrote:
Hi Kyrill,

Hi Thomas,


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?

This is ok with one ChangeLog nit.

Thanks,
Kyrill

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.

I'd say "New define_insn." instead.

         * 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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]