From: "Moore, Catherine" <Catherine_Moore@mentor.com>
To: Matthew Fortune <Matthew.Fortune@imgtec.com>,
"'gcc-patches@gcc.gnu.org' (gcc-patches@gcc.gnu.org)"
<gcc-patches@gcc.gnu.org>
Cc: "Moore, Catherine" <Catherine_Moore@mentor.com>
Subject: RE: [PATCH, MIPS] Compact branch support for MIPS32R6/MIPS64R6
Date: Fri, 14 Aug 2015 20:32:00 -0000 [thread overview]
Message-ID: <FD3DCEAC5B03E9408544A1E416F112420192C94C41@NA-MBX-04.mgc.mentorg.com> (raw)
In-Reply-To: <6D39441BF12EF246A7ABCE6654B02353211F2232@LEMAIL01.le.imgtec.org>
Hi Matthew,
> -----Original Message-----
> From: Matthew Fortune [mailto:Matthew.Fortune@imgtec.com]
> Sent: Wednesday, July 22, 2015 6:19 PM
> Subject: [PATCH, MIPS] Compact branch support for MIPS32R6/MIPS64R6
>
This patch looks really good. I have a couple of questions and a couple of nits that need to be fixed up.
> A full range of 'compact' branch instructions were introduced to MIPS
> as part of Release 6. The compact term is used to identify the fact
> that these do not have a delay slot.
>
> So how does all this work in GCC?
>
> Compact branches are used based on a branch policy. The polices are:
>
> never: Only use delay slot branches
> optimal: Do whatever is best for the current architecture. This will
> generally mean that delay slot branches will be used if the delay
> slot gets filled but otherwise a compact branch will be used. A
> special case here is that JAL and J will not be used in R6 code
> regardless of whether the delay slot could be filled.
> always: Never emit a delay slot form of a branch if a compact form exists.
> This policy cannot apply 100% as FP branches (and MSA branches when
> committed) only have delay slot forms.
>
> These user choices are combined with the features available in the chosen
> architecture and, in particular, the optimal form will get handled like
> 'never' when there are no compact branches available and will get handled
> like 'always' when there are no delay slot branches available.
>
Why did you choose to make this a user-selectable option? Why not always generated optimal?
I don't have a strong opinion about it, but the options seem to complicate things and I'm interested in your rationale.
>
> The most complicated aspect of this change is to the MIPS_CALL and
> MICROMIPS_J macros. These have been rewritten from scratch as a function
> that generates an instruction instead.
Thank you for cleaning this up. The new function is much easier to follow.
> diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
> index c3cd52d..c0f2884 100644
> --- a/gcc/config/mips/mips.c
> +++ b/gcc/config/mips/mips.c
>
> +/* Return the asm template for a call. OPERANDS are the operands,
> TARGET_OPNO
> + is the operand number of the target. SIZE_OPNO is the operand number
> of
> + the argument size operand that can optionally hold the call attributes. If
> + SIZE_OPNO is not -1 and the call is indirect, use the function symbol from
> + the call attributes to attach a R_MIPS_JALR relocation to the call.
> +
Might as well mention LINK_P here as well.
> + When generating GOT code without explicit relocation operators, all calls
> + should use assembly macros. Otherwise, all indirect calls should use "jr"
> + or "jalr"; we will arrange to restore $gp afterwards if necessary. Finally,
> + we can only generate direct calls for -mabicalls by temporarily switching
> + to non-PIC mode.
> +
> + For microMIPS jal(r), we try to generate jal(r)s when a 16-bit
> + instruction is in the delay slot of jal(r).
> +
> + Where compact branches are available, we try to use them if the delay
> slot
> + has a NOP (or equivalently delay slots were not enabled for the instruction
> + anyway). */
> +
> +const char *
> +mips_output_jump (rtx *operands, int target_opno, int size_opno, bool
> link_p)
> +{
> @@ -13038,6 +13165,59 @@ mips_output_conditional_branch (rtx_insn
> *insn, rtx *operands,
> return "";
> }
>
> +const char *
> +mips_output_equal_conditional_branch (rtx_insn* insn, rtx *operands,
> + bool inverted_p)
This function needs a comment.
diff --git a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt
index 348c6e0..84887d1 100644
--- a/gcc/config/mips/mips.opt
+++ b/gcc/config/mips/mips.opt
@@ -418,3 +418,20 @@ Driver
mload-store-pairs
Target Report Var(TARGET_LOAD_STORE_PAIRS) Init(1)
Enable load/store bonding.
+
+mcompact-branches=
+Target RejectNegative JoinedOrMissing Var(mips_cb) Report Enum(mips_cb_setting) Init(MIPS_CB_OPTIMAL)
+Specify the compact branch usage policy
+
+Enum
+Name(mips_cb_setting) Type(enum mips_cb_setting)
+Policies available for use with -mcompact-branches=:
+
+EnumValue
+Enum(mips_cb_setting) String(never) Value(MIPS_CB_NEVER)
+
+EnumValue
+Enum(mips_cb_setting) String(optimal) Value(MIPS_CB_OPTIMAL)
+
+EnumValue
+Enum(mips_cb_setting) String(always) Value(MIPS_CB_ALWAYS)
These need to be documented in invoke.texi.
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index 5bc562e..04fe6d0 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
> @@ -92,6 +92,23 @@ struct mips_cpu_info {
> /* True if we are generating position-independent VxWorks RTP code. */
> #define TARGET_RTP_PIC (TARGET_VXWORKS_RTP && flag_pic)
>
> +/* Set based on a combination of compact branch policy and ISA support.
> */
> +#define TARGET_CB_NEVER (mips_cb == MIPS_CB_NEVER \
> + || (mips_cb == MIPS_CB_OPTIMAL \
> + && !ISA_HAS_COMPACT_BRANCHES))
> +#define TARGET_CB_MAYBE (TARGET_CB_ALWAYS \
> + || (mips_cb == MIPS_CB_OPTIMAL \
> + && ISA_HAS_COMPACT_BRANCHES))
> +#define TARGET_CB_ALWAYS (mips_cb == MIPS_CB_ALWAYS \
> + || (mips_cb == MIPS_CB_OPTIMAL \
> + && !ISA_HAS_DELAY_SLOTS))
> +
I would appreciate a more detailed comment here ...
@@ -871,6 +888,10 @@ struct mips_cpu_info {
#define ISA_HAS_JR (mips_isa_rev <= 5)
+#define ISA_HAS_DELAY_SLOTS 1
If this is a placeholder for the microMIPS patch, then OK. Otherwise, what's the point?
My test run had only one test that executed with the -mcompact-branch= option. That may have been because I used RUNTESTFLAGS (the problem that we discussed at Cauldron). It looks like you put the logic in mips.exp though to have the option throw more often. In any case, I would like to see some explicit testing of the options (if they are kept). You've modified the tests to accept both the compact and non-compact forms. It would be nice to ensure that the compact form is not generated when -mcompact-branch=never.
Catherine
next prev parent reply other threads:[~2015-08-14 20:30 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-23 2:26 Matthew Fortune
2015-08-14 20:32 ` Moore, Catherine [this message]
2015-08-17 23:06 ` Matthew Fortune
2015-08-21 20:56 ` Moore, Catherine
2015-09-01 22:43 ` Matthew Fortune
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=FD3DCEAC5B03E9408544A1E416F112420192C94C41@NA-MBX-04.mgc.mentorg.com \
--to=catherine_moore@mentor.com \
--cc=Matthew.Fortune@imgtec.com \
--cc=gcc-patches@gcc.gnu.org \
/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).