From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38644 invoked by alias); 13 Jul 2016 10:54:45 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 38633 invoked by uid 89); 13 Jul 2016 10:54:44 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=tuesday, Tuesday X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 13 Jul 2016 10:54:34 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6C51F28; Wed, 13 Jul 2016 03:55:37 -0700 (PDT) Received: from e108577-lin.localnet (usa-sjc-imap-foss1.foss.arm.com [10.72.51.249]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 9DEDD3F213; Wed, 13 Jul 2016 03:54:32 -0700 (PDT) From: Thomas Preudhomme To: Kyrill Tkachov Cc: Richard Earnshaw , Ramana Radhakrishnan , gcc-patches@gcc.gnu.org Subject: Re: [PATCH, ARM 6/7, ping1] Add support for CB(N)Z and (U|S)DIV to ARMv8-M Baseline Date: Wed, 13 Jul 2016 10:54:00 -0000 Message-ID: <2207403.iO9uceAKVs@e108577-lin> User-Agent: KMail/4.13.3 (Linux/3.13.0-85-generic; KDE/4.13.3; x86_64; ; ) In-Reply-To: <57850565.9040407@foss.arm.com> References: <004601d138a3$6e832c10$4b898430$@foss.arm.com> <2740193.ae6lGT2elt@e108577-lin> <57850565.9040407@foss.arm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" X-IsSubscribed: yes X-SW-Source: 2016-07/txt/msg00709.txt.bz2 On Tuesday 12 July 2016 15:57:41 Kyrill Tkachov wrote: > 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 > >>> > >>> * 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..347b5b0a5cc0bc1e3b5020c8124d96 > >>> 8e > >>> 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..445972ce0e3fd27d4411840ff69e9e > >>> db > >>> 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..1b01ef6ce731fe3ff37c3d8c048fb9 > >>> d5 > >>> 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. Done as per your suggestion. Best regards, Thomas