public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH 1/3] MIPS: add -mcompact-branches=prefer option
@ 2021-02-05  9:53 YunQiang Su
  2021-02-05  9:53 ` [PATCH 2/3] MIPS: add builtime option for -mcompact-branches YunQiang Su
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: YunQiang Su @ 2021-02-05  9:53 UTC (permalink / raw)
  To: gcc-patches; +Cc: syq, macro, jiaxun.yang, YunQiang Su

For MIPSr6, we may wish to use compact-branches only.
Currently, we have to use `always' option, while it is mark as conflict
with pre-R6.
  cc1: error: unsupported combination: ‘mips32r2’ -mcompact-branches=always

It make some trouble for distributions to make -mcompact-branches=always
default for R6 only.

The new added `prefer' option:
   just ignored by pre-R6 target.
   do the same as `always' for R6+.
---
 gcc/config/mips/mips-opts.h                        |  3 ++-
 gcc/config/mips/mips.h                             |  6 ++++--
 gcc/config/mips/mips.opt                           |  3 +++
 gcc/doc/invoke.texi                                |  6 ++++++
 gcc/testsuite/gcc.target/mips/compact-branches-8.c | 10 ++++++++++
 gcc/testsuite/gcc.target/mips/compact-branches-9.c | 10 ++++++++++
 6 files changed, 35 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/mips/compact-branches-8.c
 create mode 100644 gcc/testsuite/gcc.target/mips/compact-branches-9.c

diff --git a/gcc/config/mips/mips-opts.h b/gcc/config/mips/mips-opts.h
index 6214849f3e1..f3804b9722b 100644
--- a/gcc/config/mips/mips-opts.h
+++ b/gcc/config/mips/mips-opts.h
@@ -51,6 +51,7 @@ enum mips_r10k_cache_barrier_setting {
 enum mips_cb_setting {
   MIPS_CB_NEVER,
   MIPS_CB_OPTIMAL,
-  MIPS_CB_ALWAYS
+  MIPS_CB_ALWAYS,
+  MIPS_CB_PREFER
 };
 #endif
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index b4a60a55d80..f8762fe6638 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -106,20 +106,22 @@ struct mips_cpu_info {
    'never' policy or the 'optimal' policy on a core that lacks
    compact branch instructions.  */
 #define TARGET_CB_NEVER (mips_cb == MIPS_CB_NEVER	\
-			 || (mips_cb == MIPS_CB_OPTIMAL \
+			 || ((mips_cb == MIPS_CB_OPTIMAL || mips_cb == MIPS_CB_PREFER) \
 			     && !ISA_HAS_COMPACT_BRANCHES))
 
 /* Compact branches may be used if the user either selects the
    'always' policy or the 'optimal' policy on a core that supports
    compact branch instructions.  */
 #define TARGET_CB_MAYBE (TARGET_CB_ALWAYS		\
-			 || (mips_cb == MIPS_CB_OPTIMAL \
+			 || ((mips_cb == MIPS_CB_OPTIMAL || mips_cb == MIPS_CB_PREFER) \
 			     && ISA_HAS_COMPACT_BRANCHES))
 
 /* Compact branches must always be generated if the user selects
    the 'always' policy or the 'optimal' policy om a core that
    lacks delay slot branch instructions.  */
 #define TARGET_CB_ALWAYS (mips_cb == MIPS_CB_ALWAYS	\
+			 || (mips_cb == MIPS_CB_PREFER \
+			     && ISA_HAS_COMPACT_BRANCHES)   \
 			 || (mips_cb == MIPS_CB_OPTIMAL \
 			     && !ISA_HAS_DELAY_SLOTS))
 
diff --git a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt
index 6af8037e9bd..f2d7550e36c 100644
--- a/gcc/config/mips/mips.opt
+++ b/gcc/config/mips/mips.opt
@@ -464,6 +464,9 @@ Enum(mips_cb_setting) String(optimal) Value(MIPS_CB_OPTIMAL)
 EnumValue
 Enum(mips_cb_setting) String(always) Value(MIPS_CB_ALWAYS)
 
+EnumValue
+Enum(mips_cb_setting) String(prefer) Value(MIPS_CB_PREFER)
+
 mloongson-mmi
 Target Mask(LOONGSON_MMI)
 Use Loongson MultiMedia extensions Instructions (MMI) instructions.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 3751bc3ac7c..9493c508d5b 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -25199,9 +25199,11 @@ and MIPS64 architectures specifically deprecate their use.
 @item -mcompact-branches=never
 @itemx -mcompact-branches=optimal
 @itemx -mcompact-branches=always
+@itemx -mcompact-branches=prefer
 @opindex mcompact-branches=never
 @opindex mcompact-branches=optimal
 @opindex mcompact-branches=always
+@opindex mcompact-branches=prefer
 These options control which form of branches will be generated.  The
 default is @option{-mcompact-branches=optimal}.
 
@@ -25215,6 +25217,10 @@ used instead.
 
 This option is supported from MIPS Release 6 onwards.
 
+The @option{-mcompact-branches=prefer} option is same with
+@option{-mcompact-branches=always} for MIPS Release 6 onwards, and
+is same with @option{-mcompact-branches=never} for pre-R6.
+
 The @option{-mcompact-branches=optimal} option will cause a delay slot
 branch to be used if one is available in the current ISA and the delay
 slot is successfully filled.  If the delay slot is not filled, a compact
diff --git a/gcc/testsuite/gcc.target/mips/compact-branches-8.c b/gcc/testsuite/gcc.target/mips/compact-branches-8.c
new file mode 100644
index 00000000000..72ffcb49cfc
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/compact-branches-8.c
@@ -0,0 +1,10 @@
+/* { dg-options "-mno-abicalls -mcompact-branches=prefer isa_rev<=5" } */
+void bar (int);
+
+void
+foo ()
+{
+  bar (1);
+}
+
+/* { dg-final { scan-assembler "\t(j|jal)\t" } } */
diff --git a/gcc/testsuite/gcc.target/mips/compact-branches-9.c b/gcc/testsuite/gcc.target/mips/compact-branches-9.c
new file mode 100644
index 00000000000..7a46b53d3e4
--- /dev/null
+++ b/gcc/testsuite/gcc.target/mips/compact-branches-9.c
@@ -0,0 +1,10 @@
+/* { dg-options "-mno-abicalls -fno-PIC -mcompact-branches=prefer isa_rev>=6" } */
+void bar (int);
+
+void
+foo ()
+{
+  bar (1);
+}
+
+/* { dg-final { scan-assembler "\t(bc|balc)\t" } } */
-- 
2.20.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 2/3] MIPS: add builtime option for -mcompact-branches
  2021-02-05  9:53 [PATCH 1/3] MIPS: add -mcompact-branches=prefer option YunQiang Su
@ 2021-02-05  9:53 ` YunQiang Su
  2021-02-16 18:41   ` Jeff Law
  2021-02-05  9:53 ` [PATCH 3/3] MIPS: fix compact-branches test FAIL for PIC default configuration YunQiang Su
  2021-02-16 19:18 ` [PATCH 1/3] MIPS: add -mcompact-branches=prefer option Jeff Law
  2 siblings, 1 reply; 11+ messages in thread
From: YunQiang Su @ 2021-02-05  9:53 UTC (permalink / raw)
  To: gcc-patches; +Cc: syq, macro, jiaxun.yang, YunQiang Su

For R6+, it allows to configure gcc to use compact branches only.
---
 gcc/config.gcc       | 18 +++++++++++++++++-
 gcc/doc/install.texi | 23 +++++++++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/gcc/config.gcc b/gcc/config.gcc
index 17fea83b2e4..7d50e7995d2 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -4743,7 +4743,7 @@ case "${target}" in
 		;;
 
 	mips*-*-*)
-		supported_defaults="abi arch arch_32 arch_64 float fpu nan fp_32 odd_spreg_32 tune tune_32 tune_64 divide llsc mips-plt synci lxc1-sxc1 madd4"
+		supported_defaults="abi arch arch_32 arch_64 float fpu nan fp_32 odd_spreg_32 tune tune_32 tune_64 divide llsc mips-plt synci lxc1-sxc1 madd4 compact-branches"
 
 		case ${with_float} in
 		"" | soft | hard)
@@ -4896,6 +4896,22 @@ case "${target}" in
 			exit 1
 			;;
 		esac
+
+		case ${with_compact_branches} in
+		never | always | optimal | prefer)
+			if test "$with_compact_branches" = "always" -a \
+				"$default_mips_arch" != "mips32r6" -a  \
+				"$default_mips_arch" != "mips64r6";then
+				echo "Compact-branch=always is not allowed for pre-R6" 1>&2
+				exit 1
+			fi
+			with_compact_branches=${with_compact_branches}
+			;;
+		*)
+			echo "Unknown compact-branches policy used in --with-compact-branches" 1>&2
+			exit 1
+			;;
+		esac
 		;;
 
 	nds32*-*-*)
diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
index 4c38244ae58..6b9520569ba 100644
--- a/gcc/doc/install.texi
+++ b/gcc/doc/install.texi
@@ -1464,6 +1464,29 @@ systems that support conditional traps).
 Division by zero checks use the break instruction.
 @end table
 
+@item --with-compact-branches=@var{policy}
+Specify how the compiler should generate code for checking for
+division by zero.  This option is only supported on the MIPS target.
+The possibilities for @var{type} are:
+@table @code
+@item optimal
+Cause a delay slot branch to be used if one is available in the
+current ISA and the delay slot is successfully filled. If the delay slot
+is not filled, a compact branch will be chosen if one is available.
+@item never
+Ensures that compact branch instructions will never be generated.
+@item always
+Ensures that a compact branch instruction will be generated if available.
+If a compact branch instruction is not available,
+a delay slot form of the branch will be used instead.
+This option is supported from MIPS Release 6 onwards.
+@item prefer
+Ensures that a compact branch instruction will be generated if available
+on MIPS Release 6 onwards.
+Simliar with @option{always} besides that this option works for pre-R6
+target, on which, this option will just be ignored.
+@end table
+
 @c If you make --with-llsc the default for additional targets,
 @c update the --with-llsc description in the MIPS section below.
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 3/3] MIPS: fix compact-branches test FAIL for PIC default configuration
  2021-02-05  9:53 [PATCH 1/3] MIPS: add -mcompact-branches=prefer option YunQiang Su
  2021-02-05  9:53 ` [PATCH 2/3] MIPS: add builtime option for -mcompact-branches YunQiang Su
@ 2021-02-05  9:53 ` YunQiang Su
  2021-02-16 18:38   ` Jeff Law
  2021-02-16 19:18 ` [PATCH 1/3] MIPS: add -mcompact-branches=prefer option Jeff Law
  2 siblings, 1 reply; 11+ messages in thread
From: YunQiang Su @ 2021-02-05  9:53 UTC (permalink / raw)
  To: gcc-patches; +Cc: syq, macro, jiaxun.yang, YunQiang Su

GCC may be configured to use PIC by default, then the test with
-mno-abicall may fail. Just add -fno-PIC option for it.
---
 gcc/testsuite/gcc.target/mips/compact-branches-5.c | 2 +-
 gcc/testsuite/gcc.target/mips/compact-branches-6.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/gcc/testsuite/gcc.target/mips/compact-branches-5.c b/gcc/testsuite/gcc.target/mips/compact-branches-5.c
index 90d312c614d..0189635bf61 100644
--- a/gcc/testsuite/gcc.target/mips/compact-branches-5.c
+++ b/gcc/testsuite/gcc.target/mips/compact-branches-5.c
@@ -1,4 +1,4 @@
-/* { dg-options "-mno-abicalls -mcompact-branches=never isa_rev>=6" } */
+/* { dg-options "-mno-abicalls -fno-PIC -mcompact-branches=never isa_rev>=6" } */
 void bar (int);
 
 void
diff --git a/gcc/testsuite/gcc.target/mips/compact-branches-6.c b/gcc/testsuite/gcc.target/mips/compact-branches-6.c
index dd35a5581bd..36180b0c76c 100644
--- a/gcc/testsuite/gcc.target/mips/compact-branches-6.c
+++ b/gcc/testsuite/gcc.target/mips/compact-branches-6.c
@@ -1,4 +1,4 @@
-/* { dg-options "-mno-abicalls -mcompact-branches=optimal isa_rev>=6" } */
+/* { dg-options "-mno-abicalls -fno-PIC -mcompact-branches=optimal isa_rev>=6" } */
 void bar (int);
 
 void
-- 
2.20.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/3] MIPS: fix compact-branches test FAIL for PIC default configuration
  2021-02-05  9:53 ` [PATCH 3/3] MIPS: fix compact-branches test FAIL for PIC default configuration YunQiang Su
@ 2021-02-16 18:38   ` Jeff Law
  0 siblings, 0 replies; 11+ messages in thread
From: Jeff Law @ 2021-02-16 18:38 UTC (permalink / raw)
  To: YunQiang Su, gcc-patches; +Cc: syq, jiaxun.yang



On 2/5/21 2:53 AM, YunQiang Su wrote:
> GCC may be configured to use PIC by default, then the test with
> -mno-abicall may fail. Just add -fno-PIC option for it.
THanks.  Installed on the trunk.

jeff


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] MIPS: add builtime option for -mcompact-branches
  2021-02-05  9:53 ` [PATCH 2/3] MIPS: add builtime option for -mcompact-branches YunQiang Su
@ 2021-02-16 18:41   ` Jeff Law
  2021-02-16 19:35     ` Maciej W. Rozycki
  0 siblings, 1 reply; 11+ messages in thread
From: Jeff Law @ 2021-02-16 18:41 UTC (permalink / raw)
  To: YunQiang Su, gcc-patches; +Cc: syq, jiaxun.yang



On 2/5/21 2:53 AM, YunQiang Su wrote:
> For R6+, it allows to configure gcc to use compact branches only.
> ---
>  gcc/config.gcc       | 18 +++++++++++++++++-
>  gcc/doc/install.texi | 23 +++++++++++++++++++++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/config.gcc b/gcc/config.gcc
> index 17fea83b2e4..7d50e7995d2 100644
> --- a/gcc/config.gcc
> +++ b/gcc/config.gcc
> @@ -4743,7 +4743,7 @@ case "${target}" in
>  		;;
>  
>  	mips*-*-*)
> -		supported_defaults="abi arch arch_32 arch_64 float fpu nan fp_32 odd_spreg_32 tune tune_32 tune_64 divide llsc mips-plt synci lxc1-sxc1 madd4"
> +		supported_defaults="abi arch arch_32 arch_64 float fpu nan fp_32 odd_spreg_32 tune tune_32 tune_64 divide llsc mips-plt synci lxc1-sxc1 madd4 compact-branches"
>  
>  		case ${with_float} in
>  		"" | soft | hard)
> @@ -4896,6 +4896,22 @@ case "${target}" in
>  			exit 1
>  			;;
>  		esac
> +
> +		case ${with_compact_branches} in
> +		never | always | optimal | prefer)
> +			if test "$with_compact_branches" = "always" -a \
> +				"$default_mips_arch" != "mips32r6" -a  \
> +				"$default_mips_arch" != "mips64r6";then
> +				echo "Compact-branch=always is not allowed for pre-R6" 1>&2
> +				exit 1
> +			fi
> +			with_compact_branches=${with_compact_branches}
> +			;;
> +		*)
> +			echo "Unknown compact-branches policy used in --with-compact-branches" 1>&2
> +			exit 1
> +			;;
> +		esac
>  		;;
>  
>  	nds32*-*-*)
> diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
> index 4c38244ae58..6b9520569ba 100644
> --- a/gcc/doc/install.texi
> +++ b/gcc/doc/install.texi
> @@ -1464,6 +1464,29 @@ systems that support conditional traps).
>  Division by zero checks use the break instruction.
>  @end table
>  
> +@item --with-compact-branches=@var{policy}
> +Specify how the compiler should generate code for checking for
> +division by zero.  This option is only supported on the MIPS target.
> +The possibilities for @var{type} are:
Is this really correct -- I would expect that the change affects
branches in general, not just checks for division by zero.

jeff


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] MIPS: add -mcompact-branches=prefer option
  2021-02-05  9:53 [PATCH 1/3] MIPS: add -mcompact-branches=prefer option YunQiang Su
  2021-02-05  9:53 ` [PATCH 2/3] MIPS: add builtime option for -mcompact-branches YunQiang Su
  2021-02-05  9:53 ` [PATCH 3/3] MIPS: fix compact-branches test FAIL for PIC default configuration YunQiang Su
@ 2021-02-16 19:18 ` Jeff Law
  2021-02-16 19:41   ` Maciej W. Rozycki
  2 siblings, 1 reply; 11+ messages in thread
From: Jeff Law @ 2021-02-16 19:18 UTC (permalink / raw)
  To: YunQiang Su, gcc-patches; +Cc: syq, jiaxun.yang



On 2/5/21 2:53 AM, YunQiang Su wrote:
> For MIPSr6, we may wish to use compact-branches only.
> Currently, we have to use `always' option, while it is mark as conflict
> with pre-R6.
>   cc1: error: unsupported combination: ‘mips32r2’ -mcompact-branches=always
>
> It make some trouble for distributions to make -mcompact-branches=always
> default for R6 only.
>
> The new added `prefer' option:
>    just ignored by pre-R6 target.
>    do the same as `always' for R6+.
> ---
>  gcc/config/mips/mips-opts.h                        |  3 ++-
>  gcc/config/mips/mips.h                             |  6 ++++--
>  gcc/config/mips/mips.opt                           |  3 +++
>  gcc/doc/invoke.texi                                |  6 ++++++
>  gcc/testsuite/gcc.target/mips/compact-branches-8.c | 10 ++++++++++
>  gcc/testsuite/gcc.target/mips/compact-branches-9.c | 10 ++++++++++
>  6 files changed, 35 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/mips/compact-branches-8.c
>  create mode 100644 gcc/testsuite/gcc.target/mips/compact-branches-9.c
I think this will be OK once the wording in patch 2/3 of this series is
fixed.

It would be helpful if you could also include a ChangeLog entry.

Jeff


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] MIPS: add builtime option for -mcompact-branches
  2021-02-16 18:41   ` Jeff Law
@ 2021-02-16 19:35     ` Maciej W. Rozycki
  2021-02-19  5:55       ` YunQiang Su
  0 siblings, 1 reply; 11+ messages in thread
From: Maciej W. Rozycki @ 2021-02-16 19:35 UTC (permalink / raw)
  To: Jeff Law; +Cc: YunQiang Su, gcc-patches, syq, jiaxun.yang

On Tue, 16 Feb 2021, Jeff Law via Gcc-patches wrote:

> > diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
> > index 4c38244ae58..6b9520569ba 100644
> > --- a/gcc/doc/install.texi
> > +++ b/gcc/doc/install.texi
> > @@ -1464,6 +1464,29 @@ systems that support conditional traps).
> >  Division by zero checks use the break instruction.
> >  @end table
> >  
> > +@item --with-compact-branches=@var{policy}
> > +Specify how the compiler should generate code for checking for
> > +division by zero.  This option is only supported on the MIPS target.
> > +The possibilities for @var{type} are:
> Is this really correct -- I would expect that the change affects
> branches in general, not just checks for division by zero.

 I wonder if we need to multiply the options here at all.  The original 
change: <https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01892.html> was 
discussed here: <https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00940.html> 
in this respect:

On Mon, 17 Aug 2015, Matthew Fortune wrote:

> > > 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.
> 
> This is down to micro-architecture decisions that different implementers may make.
> Honestly, I have not absorbed all of the rationale behind choosing one form over
> the other but our arch team have made enough comments about this to mean the support
> in the compiler is worth the extra bit of effort. I can attempt a write-up of some
> of the pipeline possibilities if you would like more detail but I'd probably have to
> refresh my mind on this with our hardware teams.

 My understanding therefore is that the original assumption that `optimal' 
will serve people best is no longer true.

 First, I think it would be good if we knew why.  I find proliferating 
variants of defaults, especially for the less obvious cases, will cause 
user confusion as one won't know what code model to expect, especially as 
(please correct me if I am wrong) we don't actually provide a way to dump 
the compiler's overridable configuration defaults.

 Second, I wonder if it makes sense to just keep things simple, and rather 
than adding `prefer' (to stand for "`always' if available"), and possibly 
`avoid' (to stand for "`never' if available"), whether we shouldn't just 
relax the checks for `always' and `never', and let them through whether 
the architecture selected provides for the option chosen or not.

 Please note that in the discussion quoted Catherine has already raised a 
concern I agree with of adding a complication here, and now we would 
complicate it even further by not only adding a fourth choice, but another 
overridable configuration default as well.

  Maciej

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/3] MIPS: add -mcompact-branches=prefer option
  2021-02-16 19:18 ` [PATCH 1/3] MIPS: add -mcompact-branches=prefer option Jeff Law
@ 2021-02-16 19:41   ` Maciej W. Rozycki
  0 siblings, 0 replies; 11+ messages in thread
From: Maciej W. Rozycki @ 2021-02-16 19:41 UTC (permalink / raw)
  To: Jeff Law; +Cc: YunQiang Su, gcc-patches, syq, jiaxun.yang

On Tue, 16 Feb 2021, Jeff Law via Gcc-patches wrote:

> I think this will be OK once the wording in patch 2/3 of this series is
> fixed.

 As I noted with 2/3 I would like to know what this extra complication is 
exactly needed for, and then whether we can't reuse the existing options.  
Once settled I think it would best be placed in the change description for 
posterity, so that discussions do not have to be chased like I had to with 
the original change (when we had a different policy on commit messages).

  Maciej

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] MIPS: add builtime option for -mcompact-branches
  2021-02-16 19:35     ` Maciej W. Rozycki
@ 2021-02-19  5:55       ` YunQiang Su
  2021-03-03 23:50         ` Maciej W. Rozycki
  0 siblings, 1 reply; 11+ messages in thread
From: YunQiang Su @ 2021-02-19  5:55 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Jeff Law, YunQiang Su, gcc-patches, Jiaxun Yang

Maciej W. Rozycki <macro@orcam.me.uk> 于2021年2月17日周三 上午3:45写道:
>
> On Tue, 16 Feb 2021, Jeff Law via Gcc-patches wrote:
>
> > > diff --git a/gcc/doc/install.texi b/gcc/doc/install.texi
> > > index 4c38244ae58..6b9520569ba 100644
> > > --- a/gcc/doc/install.texi
> > > +++ b/gcc/doc/install.texi
> > > @@ -1464,6 +1464,29 @@ systems that support conditional traps).
> > >  Division by zero checks use the break instruction.
> > >  @end table
> > >
> > > +@item --with-compact-branches=@var{policy}
> > > +Specify how the compiler should generate code for checking for
> > > +division by zero.  This option is only supported on the MIPS target.
> > > +The possibilities for @var{type} are:
> > Is this really correct -- I would expect that the change affects
> > branches in general, not just checks for division by zero.
>
>  I wonder if we need to multiply the options here at all.  The original
> change: <https://gcc.gnu.org/ml/gcc-patches/2015-07/msg01892.html> was
> discussed here: <https://gcc.gnu.org/ml/gcc-patches/2015-08/msg00940.html>
> in this respect:
>
> On Mon, 17 Aug 2015, Matthew Fortune wrote:
>
> > > > 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.
> >
> > This is down to micro-architecture decisions that different implementers may make.
> > Honestly, I have not absorbed all of the rationale behind choosing one form over
> > the other but our arch team have made enough comments about this to mean the support
> > in the compiler is worth the extra bit of effort. I can attempt a write-up of some
> > of the pipeline possibilities if you would like more detail but I'd probably have to
> > refresh my mind on this with our hardware teams.
>
>  My understanding therefore is that the original assumption that `optimal'
> will serve people best is no longer true.
>

I guess that `optimal' can still produce the best performance, while
the delay slot
make MIPS quite differnent with other architectures.
And the hardware engineers seems hate it also.

And we expect that MIPS can have as few as possible differnece delta
with other major architectures,
to ultily all of new framworks of community.

>  First, I think it would be good if we knew why.  I find proliferating
> variants of defaults, especially for the less obvious cases, will cause
> user confusion as one won't know what code model to expect, especially as
> (please correct me if I am wrong) we don't actually provide a way to dump
> the compiler's overridable configuration defaults.
>

So, should we provide a predefined macro for it?

>  Second, I wonder if it makes sense to just keep things simple, and rather
> than adding `prefer' (to stand for "`always' if available"), and possibly
> `avoid' (to stand for "`never' if available"), whether we shouldn't just
> relax the checks for `always' and `never', and let them through whether
> the architecture selected provides for the option chosen or not.
>

relax the `always' is what I would like to do first.
But I was afread to break some complatiable.

>  Please note that in the discussion quoted Catherine has already raised a
> concern I agree with of adding a complication here, and now we would
> complicate it even further by not only adding a fourth choice, but another
> overridable configuration default as well.

I am still concern about whether we should just set the `always' as default.
My short team plan is to set it default for Debian r6 Port.
So, at least, I wish that we can provide a buildtime option for other need.

>
>   Maciej

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] MIPS: add builtime option for -mcompact-branches
  2021-02-19  5:55       ` YunQiang Su
@ 2021-03-03 23:50         ` Maciej W. Rozycki
  2021-03-04  3:24           ` YunQiang Su
  0 siblings, 1 reply; 11+ messages in thread
From: Maciej W. Rozycki @ 2021-03-03 23:50 UTC (permalink / raw)
  To: YunQiang Su; +Cc: Jeff Law, YunQiang Su, gcc-patches, Jiaxun Yang

On Fri, 19 Feb 2021, YunQiang Su wrote:

> >  My understanding therefore is that the original assumption that `optimal'
> > will serve people best is no longer true.
> >
> 
> I guess that `optimal' can still produce the best performance, while
> the delay slot
> make MIPS quite differnent with other architectures.
> And the hardware engineers seems hate it also.

 Right, but what does it have to do with compiler defaults?  Given what we 
have available in hardware we want the best results possible, except for 
research or special use cases (such as GAS's `-minsn32' option with 
microMIPS code).  I would like to understand what the use case is here.

> And we expect that MIPS can have as few as possible differnece delta
> with other major architectures,
> to ultily all of new framworks of community.

 Well, machine code is inherently architecture-specific, so you can't 
have a single template that fits all.  The difference betwen processor 
architectures is more than just the bit patterns for individual opcodes
and operand encodings (and the corresponding mnemonics and syntax for 
the assembly language).

 For example one of the major architectures is ARM, which has conditions 
encoded with all the instructions.  And you cant mimic it with other ISAs.  
Similarly Power has 8 sets of condition codes and dedicated instructions 
to make ALU operations between these codes.  You can't do those elsewhere 
either.  Well, the MIPS or RISC-V ISAs do not have condition codes at all. 
And x86 is not a load-store architecture at all, so you'll see operations 
made directly on memory, as a destination even (let's ignore the even more 
arcane original 32-bit instruction set).

 These are all considered major architectures nowadays.

 So we have got the MIPS ISA and its delay slots.  Some subsets/variations 
of the ISA have already either greatly reduced their use or eliminated 
them completely, but we went into great lengths with GCC to produce good 
code making use of these delay slots, so I think it would be a shame to 
get this effort wasted on one hand, and MIPS code put at a disadvantage 
due to cycles wasted for pipeline stalls that could be avoided if delay 
slots were scheduled -- on the other.

> >  First, I think it would be good if we knew why.  I find proliferating
> > variants of defaults, especially for the less obvious cases, will cause
> > user confusion as one won't know what code model to expect, especially as
> > (please correct me if I am wrong) we don't actually provide a way to dump
> > the compiler's overridable configuration defaults.
> >
> 
> So, should we provide a predefined macro for it?

 I've been thinking more along `gcc -v --version' dumping the invocation 
of `configure' used, but I have to correct myself here in that it already 
happens, so nothing to do.  I'm not sure why I forgot it and/or could not 
have figured it out previously.  Sorry about the confusion.

> >  Second, I wonder if it makes sense to just keep things simple, and rather
> > than adding `prefer' (to stand for "`always' if available"), and possibly
> > `avoid' (to stand for "`never' if available"), whether we shouldn't just
> > relax the checks for `always' and `never', and let them through whether
> > the architecture selected provides for the option chosen or not.
> >
> 
> relax the `always' is what I would like to do first.
> But I was afread to break some complatiable.

 Hmm, honestly I don't think there could be any compatibility to care of 
here given that the compiler currently refuses to run with such an option 
combination.  Nobody may have relied on that then and the extra protection 
given is in my opinion a bit excessive.  Garbage in, garbage out: you get 
what you have requested.  Our usual policy with irrelevant options has 
been to silently ignore them, which helps users override Makefile defaults 
by just having CFLAGS, etc. appended to whatever the defaults are.

> >  Please note that in the discussion quoted Catherine has already raised a
> > concern I agree with of adding a complication here, and now we would
> > complicate it even further by not only adding a fourth choice, but another
> > overridable configuration default as well.
> 
> I am still concern about whether we should just set the `always' as default.
> My short team plan is to set it default for Debian r6 Port.
> So, at least, I wish that we can provide a buildtime option for other need.

 You're free with what you do with your distribution, although it seems to 
me like it's going to be a performance regression.  I suggest that you do 
some benchmarking with real code and hardware before you decide.  Maybe 
you can prove me wrong and there will be no loss of any significance.

  Maciej

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/3] MIPS: add builtime option for -mcompact-branches
  2021-03-03 23:50         ` Maciej W. Rozycki
@ 2021-03-04  3:24           ` YunQiang Su
  0 siblings, 0 replies; 11+ messages in thread
From: YunQiang Su @ 2021-03-04  3:24 UTC (permalink / raw)
  To: Maciej W. Rozycki; +Cc: Jeff Law, YunQiang Su, gcc-patches, Jiaxun Yang

Maciej W. Rozycki <macro@orcam.me.uk> 于2021年3月4日周四 上午7:50写道:
>
> On Fri, 19 Feb 2021, YunQiang Su wrote:
>
> > >  My understanding therefore is that the original assumption that `optimal'
> > > will serve people best is no longer true.
> > >
> >
> > I guess that `optimal' can still produce the best performance, while
> > the delay slot
> > make MIPS quite differnent with other architectures.
> > And the hardware engineers seems hate it also.
>
>  Right, but what does it have to do with compiler defaults?  Given what we
> have available in hardware we want the best results possible, except for
> research or special use cases (such as GAS's `-minsn32' option with
> microMIPS code).  I would like to understand what the use case is here.
>

I want to give the hardware designer the possibility to remove the
delay slot branch
instructions in future (maybe in a long term).

> > And we expect that MIPS can have as few as possible differnece delta
> > with other major architectures,
> > to ultily all of new framworks of community.
>
>  Well, machine code is inherently architecture-specific, so you can't
> have a single template that fits all.  The difference betwen processor
> architectures is more than just the bit patterns for individual opcodes
> and operand encodings (and the corresponding mnemonics and syntax for
> the assembly language).
>

Delay slot makes some troubles for newly written binary tools.
Lots of people are not used to delay slot, and then they may not
consider MIPS in some new work.
We cannot expect to much the support from them, so we have to reduce the delta.

>  For example one of the major architectures is ARM, which has conditions
> encoded with all the instructions.  And you cant mimic it with other ISAs.
> Similarly Power has 8 sets of condition codes and dedicated instructions
> to make ALU operations between these codes.  You can't do those elsewhere
> either.  Well, the MIPS or RISC-V ISAs do not have condition codes at all.
> And x86 is not a load-store architecture at all, so you'll see operations
> made directly on memory, as a destination even (let's ignore the even more
> arcane original 32-bit instruction set).
>
>  These are all considered major architectures nowadays.

Yes. "nowadays".
How about the future?
Since the MIPS is the only architecture which has delay slot, will it
be ”no-major“?

>
>  So we have got the MIPS ISA and its delay slots.  Some subsets/variations
> of the ISA have already either greatly reduced their use or eliminated
> them completely, but we went into great lengths with GCC to produce good
> code making use of these delay slots, so I think it would be a shame to

Yes. There will not be a  problem for GCC itself, while there are more and more
other tools, open source or commercial.
Since MIPS is not the de-facto majority, it is not so easy to ask them
to support MIPS.

> get this effort wasted on one hand, and MIPS code put at a disadvantage
> due to cycles wasted for pipeline stalls that could be avoided if delay
> slots were scheduled -- on the other.
>

I have some tests on I6500. The performance of delay-branch and compact-branch
are almost the same. It will not be a performance regression.

> > >  First, I think it would be good if we knew why.  I find proliferating
> > > variants of defaults, especially for the less obvious cases, will cause
> > > user confusion as one won't know what code model to expect, especially as
> > > (please correct me if I am wrong) we don't actually provide a way to dump
> > > the compiler's overridable configuration defaults.
> > >
> >
> > So, should we provide a predefined macro for it?
>
>  I've been thinking more along `gcc -v --version' dumping the invocation
> of `configure' used, but I have to correct myself here in that it already
> happens, so nothing to do.  I'm not sure why I forgot it and/or could not
> have figured it out previously.  Sorry about the confusion.
>

nop.

> > >  Second, I wonder if it makes sense to just keep things simple, and rather
> > > than adding `prefer' (to stand for "`always' if available"), and possibly
> > > `avoid' (to stand for "`never' if available"), whether we shouldn't just
> > > relax the checks for `always' and `never', and let them through whether
> > > the architecture selected provides for the option chosen or not.
> > >
> >
> > relax the `always' is what I would like to do first.
> > But I was afread to break some complatiable.
>
>  Hmm, honestly I don't think there could be any compatibility to care of
> here given that the compiler currently refuses to run with such an option
> combination.  Nobody may have relied on that then and the extra protection
> given is in my opinion a bit excessive.  Garbage in, garbage out: you get
> what you have requested.  Our usual policy with irrelevant options has
> been to silently ignore them, which helps users override Makefile defaults
> by just having CFLAGS, etc. appended to whatever the defaults are.
>

Again, it is not so easy to ask ALL packages to have MIPS dirty
patches just for a CFLAGS.

> > >  Please note that in the discussion quoted Catherine has already raised a
> > > concern I agree with of adding a complication here, and now we would
> > > complicate it even further by not only adding a fourth choice, but another
> > > overridable configuration default as well.
> >
> > I am still concern about whether we should just set the `always' as default.
> > My short team plan is to set it default for Debian r6 Port.
> > So, at least, I wish that we can provide a buildtime option for other need.
>
>  You're free with what you do with your distribution, although it seems to
> me like it's going to be a performance regression.  I suggest that you do

With some tests, there are no performance regression at all on I6500.

> some benchmarking with real code and hardware before you decide.  Maybe
> you can prove me wrong and there will be no loss of any significance.

#include "stdio.h"
#include "perf_func.h"
#include <mips/m64c0.h>

int PerfCnt[4];

__attribute__((noinline)) int add_1(int x) {
        return x+1;
}

int main(){
        int a;
        mipsperf_switch_group();
        for (int i=0; i<1000000; i++){
                a+=add_1(a);
        }
        PerfCnt[0] = mips32_get_c0(C0_PERFCNT0);
        PerfCnt[1] = mips32_get_c0(C0_PERFCNT1);
        return a;
}

This is an example of my tests. It has almost the same performance with
   -mcompact-branches=always
  -mcompact-branches=optimal

I guess you may have more tests.

>
>   Maciej

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-03-04  3:24 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-05  9:53 [PATCH 1/3] MIPS: add -mcompact-branches=prefer option YunQiang Su
2021-02-05  9:53 ` [PATCH 2/3] MIPS: add builtime option for -mcompact-branches YunQiang Su
2021-02-16 18:41   ` Jeff Law
2021-02-16 19:35     ` Maciej W. Rozycki
2021-02-19  5:55       ` YunQiang Su
2021-03-03 23:50         ` Maciej W. Rozycki
2021-03-04  3:24           ` YunQiang Su
2021-02-05  9:53 ` [PATCH 3/3] MIPS: fix compact-branches test FAIL for PIC default configuration YunQiang Su
2021-02-16 18:38   ` Jeff Law
2021-02-16 19:18 ` [PATCH 1/3] MIPS: add -mcompact-branches=prefer option Jeff Law
2021-02-16 19:41   ` Maciej W. Rozycki

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).