From: Thomas Preudhomme <thomas.preudhomme@foss.arm.com>
To: Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>
Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>,
Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>,
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 [thread overview]
Message-ID: <2207403.iO9uceAKVs@e108577-lin> (raw)
In-Reply-To: <57850565.9040407@foss.arm.com>
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 <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..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
prev parent reply other threads:[~2016-07-13 10:54 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-17 8:18 [PATCH, ARM 6/6] " Thomas Preud'homme
2016-05-17 10:14 ` [PATCH, ARM 6/7, ping1] " Thomas Preudhomme
2016-05-20 13:23 ` Kyrill Tkachov
2016-07-06 16:55 ` Thomas Preudhomme
2016-07-12 10:27 ` Thomas Preudhomme
2016-07-12 14:57 ` Kyrill Tkachov
2016-07-13 10:54 ` Thomas Preudhomme [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2207403.iO9uceAKVs@e108577-lin \
--to=thomas.preudhomme@foss.arm.com \
--cc=Richard.Earnshaw@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=kyrylo.tkachov@foss.arm.com \
--cc=ramana.radhakrishnan@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).