| Message ID | 3124fcb08e433bc230f53fae9d0e7bd6998f8538.1541607095.git.noring@nocrew.org |
|---|---|
| State | New |
| Headers | show |
| Series | MIPS: Add `-mfix-r5900' option for the R5900 short loop erratum | expand |
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
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
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
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
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
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
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
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
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(-)