MIPS: Add `-mfix-r5900' option for the R5900 short loop erratum

Message ID 3124fcb08e433bc230f53fae9d0e7bd6998f8538.1541607095.git.noring@nocrew.org
State New
Headers show
Series
  • MIPS: Add `-mfix-r5900' option for the R5900 short loop erratum
Related show

Commit Message

Fredrik Noring Nov. 7, 2018, 4:28 p.m.
The short loop bug under certain conditions causes loops to
execute only once or twice, due to a hardware bug in the R5900 chip.

`-march=r5900' already enables the R5900 short loop workaround.
However, the R5900 ISA and most other MIPS ISAs are mutually
exclusive since R5900-specific instructions are generated as well.

The `-mfix-r5900' option can be used in combination with e.g.
`-mips2' or `-mips3' to generate generic MIPS binaries that also
work with the R5900 target.  The workaround is implemented by GAS
rather than by GCC.

The following small `shortloop.c' file has been used as a test
with GCC 8.2.0:

void shortloop(void)
{
    __asm__ __volatile__ (
	"	li $3, 300\n"
	"loop:\n"
	"	addi $3, -1\n"
	"	addi $4, -1\n"
	"	bne $3, $0, loop\n"
	"	li $4, 3\n"
	::);
}

The following six combinations have been tested:

% mipsr5900el-unknown-linux-gnu-gcc -O1 -c shortloop.c
% mipsr5900el-unknown-linux-gnu-gcc -O1 -c shortloop.c -mfix-r5900
% mipsr5900el-unknown-linux-gnu-gcc -O1 -c shortloop.c -mno-fix-r5900

% mipsr4000el-unknown-linux-gnu-gcc -O1 -c shortloop.c
% mipsr4000el-unknown-linux-gnu-gcc -O1 -c shortloop.c -mfix-r5900
% mipsr4000el-unknown-linux-gnu-gcc -O1 -c shortloop.c -mno-fix-r5900

The R5900 short loop erratum is corrected in exactly three cases:

1. for the target `mipsr5900el' by default;

2. for the target `mipsr5900el' with `-mfix-r5900';

3. for any other MIPS target (e.g. `mipsr4000el') with `-mfix-r5900'.

In all other cases the correction is not made.

	* gcc/config/mips/mips.c (mips_reorg_process_insns)
	  (mips_option_override): Default to working around R5900
	  errata only if the processor was selected explicitly.
	* gcc/config/mips/mips.h: Declare `mfix-r5900' and
	  `mno-fix-r5900'.
	* gcc/config/mips/mips.opt: Define MASK_FIX_R5900.
	* gcc/doc/invoke.texi: Document the R5900, `mfix-r5900' and
	  `mno-fix-r5900'.
---
 gcc/config/mips/mips.c   | 14 ++++++++++----
 gcc/config/mips/mips.h   |  1 +
 gcc/config/mips/mips.opt |  4 ++++
 gcc/doc/invoke.texi      | 14 +++++++++++++-
 4 files changed, 28 insertions(+), 5 deletions(-)

Comments

Maciej W. Rozycki Nov. 8, 2018, 10:45 p.m. | #1
Hi Fredrik,

 Thank you for your submission.  Your change looks very good to me, except 
for a bunch of minor nits in your proposed ChangeLog entry.

> 	* gcc/config/mips/mips.c (mips_reorg_process_insns)
> 	  (mips_option_override): Default to working around R5900
> 	  errata only if the processor was selected explicitly.

 I think this only describes the `mips_option_override' part.  ChangeLog 
entries are best concise, so how about just:

	* config/mips/mips.c (mips_reorg_process_insns)
	(mips_option_override): Handle `-mfix-r5900'.

?

> 	* gcc/config/mips/mips.h: Declare `mfix-r5900' and
> 	  `mno-fix-r5900'.

 This has to be:

	* config/mips/mips.h (ASM_SPEC): Add `mfix-r5900' and 
	`mno-fix-r5900'.

as you're adding to the ASM_SPEC definition.  Also your patch lines are 
way off and I had to seach for the place this change applies to -- have 
you been using current upstream trunk with this change?

> 	* gcc/config/mips/mips.opt: Define MASK_FIX_R5900.

 Likewise:

	* config/mips/mips.opt (mfix-r5900): New option.

as it's an entry for `mfix-r5900' that you're adding.

> 	* gcc/doc/invoke.texi: Document the R5900, `mfix-r5900' and
> 	  `mno-fix-r5900'.

 It looks like missing bits, did you mean:

	* doc/invoke.texi: Document the `r5900' processor name, and 
	`-mfix-r5900' and `-mno-fix-r5900' options.

?

 All these entries apply to the gcc/ChangeLog rather than the top-level 
one, so please drop the leading gcc/ part of the paths, and indicate it at 
the beginning instead.

 Please also fix indentation throughout; the subsequent lines of entries 
are horizontally aligned with the asterisk above and not indented any 
further.

 Please resubmit your change with the ChangeLog entry updated.  
Regrettably I have no authority to approve your submission, but hopefully 
a maintainer will so that it can make it to GCC 9.

  Maciej
Fredrik Noring Nov. 9, 2018, 6:43 p.m. | #2
Many thanks for your prompt review, Maciej!

> > 	* gcc/config/mips/mips.c (mips_reorg_process_insns)
> > 	  (mips_option_override): Default to working around R5900
> > 	  errata only if the processor was selected explicitly.
> 
>  I think this only describes the `mips_option_override' part.  ChangeLog 
> entries are best concise, so how about just:
> 
> 	* config/mips/mips.c (mips_reorg_process_insns)
> 	(mips_option_override): Handle `-mfix-r5900'.
> 
> ?

Done!

> > 	* gcc/config/mips/mips.h: Declare `mfix-r5900' and
> > 	  `mno-fix-r5900'.
> 
>  This has to be:
> 
> 	* config/mips/mips.h (ASM_SPEC): Add `mfix-r5900' and 
> 	`mno-fix-r5900'.
> 
> as you're adding to the ASM_SPEC definition.

Yes, done!

> Also your patch lines are 
> way off and I had to seach for the place this change applies to -- have 
> you been using current upstream trunk with this change?

Hmm... I thought so, but obviously 2 November was a week old. I'm sorry
about that, I have rebased v2 now. [ As mentioned in the commit message,
the test was done with GCC 8.2.0 since there was an unrelated problem
compiling some sanitizer component at the time. I will try to test a
current GCC version during the weekend. ]

> > 	* gcc/config/mips/mips.opt: Define MASK_FIX_R5900.
> 
>  Likewise:
> 
> 	* config/mips/mips.opt (mfix-r5900): New option.
> 
> as it's an entry for `mfix-r5900' that you're adding.

Done!

> > 	* gcc/doc/invoke.texi: Document the R5900, `mfix-r5900' and
> > 	  `mno-fix-r5900'.
> 
>  It looks like missing bits, did you mean:
> 
> 	* doc/invoke.texi: Document the `r5900' processor name, and 
> 	`-mfix-r5900' and `-mno-fix-r5900' options.
> 
> ?

Yes, done!

>  All these entries apply to the gcc/ChangeLog rather than the top-level 
> one, so please drop the leading gcc/ part of the paths, and indicate it at 
> the beginning instead.

Done!

>  Please also fix indentation throughout; the subsequent lines of entries 
> are horizontally aligned with the asterisk above and not indented any 
> further.

Ah, yes, of course.

>  Please resubmit your change with the ChangeLog entry updated.  
> Regrettably I have no authority to approve your submission, but hopefully 
> a maintainer will so that it can make it to GCC 9.

I will post v2 shortly. Let's hope for the best. Thanks again!

Fredrik
Matthew Fortune Nov. 9, 2018, 10:30 p.m. | #3
Hi Fredrik,

> Many thanks for your prompt review, Maciej!

Maciej, I am equally grateful for you stepping in to get Fredrik's
contribution reviewed.  I have taken a look through and have no
additional comments.  I'm going to be travelling this weekend so
I'd like to ask Maciej to take a quick look at the v2 version but
otherwise pre-approve it.

Has your copyright assignment come through for GCC? I can't access
the copyright info at the moment to check myself. Obviously that will
be a blocker otherwise.

I hope you do manage to work through other r5900 issues and if so the
changes will be most welcome.

Thanks,
Matthew
Fredrik Noring Nov. 10, 2018, 9:32 a.m. | #4
Thank you for your reviews, Matthew,

> Has your copyright assignment come through for GCC? I can't access
> the copyright info at the moment to check myself. Obviously that will
> be a blocker otherwise.

I filled in and posted request-assign.future to the FSF 1 October, but I
have not heard from them since.

> I hope you do manage to work through other r5900 issues and if so the
> changes will be most welcome.

Thank you! Maciej has been most helpful also with Binutils, Glibc, QEMU
and the Linux kernel.

Fredrik
Fredrik Noring Nov. 11, 2018, 12:31 p.m. | #5
Hi Matthew,

> [ As mentioned in the commit message, the test was done with GCC 8.2.0
> since there was an unrelated problem compiling some sanitizer component
> at the time. I will try to test a current GCC version during the weekend. ]

The compilation problem, more specifically, is

In file included from ../../../../libsanitizer/sanitizer_common/sanitizer_platform_limits_linux.cc:20:
../../../../libsanitizer/sanitizer_common/sanitizer_internal_defs.h:339:72: error: narrowing conversion of ‘-1’ from ‘int’ to ‘unsigned int’  [-Wnarrowing]
  339 |     typedef char IMPL_PASTE(assertion_failed_##_, line)[2*(int)(pred)-1]
      |                                                                        ^
../../../../libsanitizer/sanitizer_common/sanitizer_internal_defs.h:333:30: note: in expansion of macro ‘IMPL_COMPILER_ASSERT’
  333 | #define COMPILER_CHECK(pred) IMPL_COMPILER_ASSERT(pred, __LINE__)
      |                              ^~~~~~~~~~~~~~~~~~~~
../../../../libsanitizer/sanitizer_common/sanitizer_platform_limits_linux.cc:71:1: note: in expansion of macro ‘COMPILER_CHECK’
   71 | COMPILER_CHECK(struct_kernel_stat_sz == sizeof(struct stat));
      | ^~~~~~~~~~~~~~
../../../../libsanitizer/sanitizer_common/sanitizer_internal_defs.h:339:72: warning: size of array is not an integral constant-expression [-Wpedantic]
  339 |     typedef char IMPL_PASTE(assertion_failed_##_, line)[2*(int)(pred)-1]
      |                                                                        ^
../../../../libsanitizer/sanitizer_common/sanitizer_internal_defs.h:333:30: note: in expansion of macro ‘IMPL_COMPILER_ASSERT’
  333 | #define COMPILER_CHECK(pred) IMPL_COMPILER_ASSERT(pred, __LINE__)
      |                              ^~~~~~~~~~~~~~~~~~~~
../../../../libsanitizer/sanitizer_common/sanitizer_platform_limits_linux.cc:71:1: note: in expansion of macro ‘COMPILER_CHECK’
   71 | COMPILER_CHECK(struct_kernel_stat_sz == sizeof(struct stat));
      | ^~~~~~~~~~~~~~
../../../../libsanitizer/sanitizer_common/sanitizer_internal_defs.h:339:72: error: size of array ‘assertion_failed__71’ is negative
  339 |     typedef char IMPL_PASTE(assertion_failed_##_, line)[2*(int)(pred)-1]
      |                                                                        ^
../../../../libsanitizer/sanitizer_common/sanitizer_internal_defs.h:333:30: note: in expansion of macro ‘IMPL_COMPILER_ASSERT’
  333 | #define COMPILER_CHECK(pred) IMPL_COMPILER_ASSERT(pred, __LINE__)
      |                              ^~~~~~~~~~~~~~~~~~~~
../../../../libsanitizer/sanitizer_common/sanitizer_platform_limits_linux.cc:71:1: note: in expansion of macro ‘COMPILER_CHECK’
   71 | COMPILER_CHECK(struct_kernel_stat_sz == sizeof(struct stat));
      | ^~~~~~~~~~~~~~

on commit a1054504a5da (origin/trunk as of now). The host is ppc64le with
gcc-7.3.0 (Gentoo 7.3.0-r3 p1.4) and the target is mipsr5900el-unknown-
linux-gnu.

Fredrik
Maciej W. Rozycki Nov. 13, 2018, 12:59 a.m. | #6
On Sun, 11 Nov 2018, Fredrik Noring wrote:

> ../../../../libsanitizer/sanitizer_common/sanitizer_platform_limits_linux.cc:71:1: note: in expansion of macro ‘COMPILER_CHECK’
>    71 | COMPILER_CHECK(struct_kernel_stat_sz == sizeof(struct stat));
>       | ^~~~~~~~~~~~~~

 I guess `struct_kernel_stat_sz' and `sizeof(struct stat)' do not match.  
You may try making a preprocessed source with the same GCC invocation 
(possibly with `-dD' added if needed) to see how these items have been 
defined in your build environment.  This may reveal something obvious.

 Also unless you realise the problem is due to misconfiguration, please 
file it in GCC bugzilla as a GCC 9 regression (since as you say 8.2.0 
builds just fine in your environment).  We don't want things to break with 
new releases.

  Maciej
Hans-Peter Nilsson Nov. 15, 2018, 4:39 a.m. | #7
On Tue, 13 Nov 2018, Maciej W. Rozycki wrote:

> On Sun, 11 Nov 2018, Fredrik Noring wrote:
>
> > ../../../../libsanitizer/sanitizer_common/sanitizer_platform_limits_linux.cc:71:1: note: in expansion of macro ?COMPILER_CHECK?
> >    71 | COMPILER_CHECK(struct_kernel_stat_sz == sizeof(struct stat));
> >       | ^~~~~~~~~~~~~~
>
>  I guess `struct_kernel_stat_sz' and `sizeof(struct stat)' do not match.
> You may try making a preprocessed source with the same GCC invocation
> (possibly with `-dD' added if needed) to see how these items have been
> defined in your build environment.  This may reveal something obvious.
>
>  Also unless you realise the problem is due to misconfiguration, please
> file it in GCC bugzilla as a GCC 9 regression (since as you say 8.2.0
> builds just fine in your environment).  We don't want things to break with
> new releases.

This sounds familiar.

Perhaps the local edits I made for sanitizer support for MIPS
have been overwritten by the upstream import?  I know I made a
boo-boo and didn't "upstream" that.

brgds, H-P

Patch

diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c
index ea2fae1d6db..5763ce21427 100644
--- a/gcc/config/mips/mips.c
+++ b/gcc/config/mips/mips.c
@@ -18881,13 +18881,13 @@  mips_reorg_process_insns (void)
   if (crtl->profile)
     cfun->machine->all_noreorder_p = false;
 
-  /* Code compiled with -mfix-vr4120, -mfix-rm7000 or -mfix-24k can't be
-     all noreorder because we rely on the assembler to work around some
-     errata.  The R5900 too has several bugs.  */
+  /* Code compiled with -mfix-vr4120, -mfix-r5900, -mfix-rm7000 or
+     -mfix-24k can't be all noreorder because we rely on the assembler
+     to work around some errata.  The R5900 target has several bugs.  */
   if (TARGET_FIX_VR4120
       || TARGET_FIX_RM7000
       || TARGET_FIX_24K
-      || TARGET_MIPS5900)
+      || TARGET_FIX_R5900)
     cfun->machine->all_noreorder_p = false;
 
   /* The same is true for -mfix-vr4130 if we might generate MFLO or
@@ -20244,6 +20244,12 @@  mips_option_override (void)
       && strcmp (mips_arch_info->name, "r4400") == 0)
     target_flags |= MASK_FIX_R4400;
 
+  /* Default to working around R5900 errata only if the processor
+     was selected explicitly.  */
+  if ((target_flags_explicit & MASK_FIX_R5900) == 0
+      && strcmp (mips_arch_info->name, "r5900") == 0)
+    target_flags |= MASK_FIX_R5900;
+
   /* Default to working around R10000 errata only if the processor
      was selected explicitly.  */
   if ((target_flags_explicit & MASK_FIX_R10000) == 0
diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h
index 32a88edc910..7dd19fc6f2d 100644
--- a/gcc/config/mips/mips.h
+++ b/gcc/config/mips/mips.h
@@ -1363,6 +1363,7 @@  struct mips_cpu_info {
 %{mmsa} %{mno-msa} \
 %{msmartmips} %{mno-smartmips} \
 %{mmt} %{mno-mt} \
+%{mfix-r5900} %{mno-fix-r5900} \
 %{mfix-rm7000} %{mno-fix-rm7000} \
 %{mfix-vr4120} %{mfix-vr4130} \
 %{mfix-24k} \
diff --git a/gcc/config/mips/mips.opt b/gcc/config/mips/mips.opt
index 5a9f255fe20..427ac4913fc 100644
--- a/gcc/config/mips/mips.opt
+++ b/gcc/config/mips/mips.opt
@@ -165,6 +165,10 @@  mfix-r4400
 Target Report Mask(FIX_R4400)
 Work around certain R4400 errata.
 
+mfix-r5900
+Target Report Mask(FIX_R5900)
+Work around the R5900 short loop erratum.
+
 mfix-rm7000
 Target Report Var(TARGET_FIX_RM7000)
 Work around certain RM7000 errata.
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index e290128f535..c9846d96304 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -939,6 +939,7 @@  Objective-C and Objective-C++ Dialects}.
 -mmad  -mno-mad  -mimadd  -mno-imadd  -mfused-madd  -mno-fused-madd  -nocpp @gol
 -mfix-24k  -mno-fix-24k @gol
 -mfix-r4000  -mno-fix-r4000  -mfix-r4400  -mno-fix-r4400 @gol
+-mfix-r5900  -mno-fix-r5900 @gol
 -mfix-r10000  -mno-fix-r10000  -mfix-rm7000  -mno-fix-rm7000 @gol
 -mfix-vr4120  -mno-fix-vr4120 @gol
 -mfix-vr4130  -mno-fix-vr4130  -mfix-sb1  -mno-fix-sb1 @gol
@@ -20804,7 +20805,8 @@  The processor names are:
 @samp{orion},
 @samp{p5600}, @samp{p6600},
 @samp{r2000}, @samp{r3000}, @samp{r3900}, @samp{r4000}, @samp{r4400},
-@samp{r4600}, @samp{r4650}, @samp{r4700}, @samp{r6000}, @samp{r8000},
+@samp{r4600}, @samp{r4650}, @samp{r4700}, @samp{r5900},
+@samp{r6000}, @samp{r8000},
 @samp{rm7000}, @samp{rm9000},
 @samp{r10000}, @samp{r12000}, @samp{r14000}, @samp{r16000},
 @samp{sb1},
@@ -21578,6 +21580,16 @@  branch-likely instructions.  @option{-mfix-r10000} is the default when
 @option{-march=r10000} is used; @option{-mno-fix-r10000} is the default
 otherwise.
 
+@item -mfix-r5900
+@itemx -mno-fix-r5900
+@opindex mfix-r5900
+Do not attempt to schedule the preceding instruction into the delay slot
+of a branch instruction placed at the end of a short loop of six
+instructions or fewer and always schedule a @code{nop} instruction there
+instead.  The short loop bug under certain conditions causes loops to
+execute only once or twice, due to a hardware bug in the R5900 chip.  The
+workaround is implemented by the assembler rather than by GCC@.
+
 @item -mfix-rm7000
 @itemx -mno-fix-rm7000
 @opindex mfix-rm7000