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.preudho...@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.preudho...@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.preudho...@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, ThomasThanks, 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.preudho...@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
