> From: [email protected] [mailto:gcc-patches-
> [email protected]] On Behalf Of Toma Tabacu
> Sent: 04 November 2016 15:25
> To: Matthew Fortune; [email protected]
> Cc: [email protected]
> Subject: RE: [PATCH,testsuite] MIPS: Downgrade R6 to R5 if tests need branch-
> likely instructions.
>
> > From: Matthew Fortune
> > Sent: 03 November 2016 13:07
> > To: Toma Tabacu; [email protected]
> > Cc: [email protected]
> > Subject: RE: [PATCH,testsuite] MIPS: Downgrade R6 to R5 if tests need
> > branch-likely instructions.
> >
> > Toma Tabacu <[email protected]> writes:
> > > The gcc.target/mips/wrap-delay.c test was failing on mips-img-*
> > > toolchains because it was using -mbranch-likely with an R6 target, and
> > > branch- likely instructions were removed in R6.
> > >
> > > This patch makes the testsuite downgrade to R5 if the -mbranch-likely
> > > option is present and we're targeting R6.
> > >
> > > Tested with mips-img-elf and mips-img-linux-gnu.
> >
> > Hi Toma,
> >
> > Welcome to GCC development, thanks for your first patch. As you can see
> > from Catherine's reply the change looks good. I'll just cover some
> > housekeeping issues...
> >
> > > gcc/testsuite/
> > > * gcc.target/mips/mips.exp: Add check for -mbranch-likely in
> > > condition for R5 downgrade.
> >
> > Changelogs are an art form which will take some getting used to. This is
> > almost there but needs to reference the function affected.
> >
> > * gcc.target/mips/mips.exp (mips-dg-options): Downgrade to R5
> > for -mbranch-likely and infer -mno-branch-likely for R6.
> >
> > I have extended the comment as well as there is an additional change
> > needed for this patch ideally.
> >
> > > diff --git a/gcc/testsuite/gcc.target/mips/mips.exp
> > > b/gcc/testsuite/gcc.target/mips/mips.exp
> > > index 7c24140..382d69c 100644
> > > --- a/gcc/testsuite/gcc.target/mips/mips.exp
> > > +++ b/gcc/testsuite/gcc.target/mips/mips.exp
> > > @@ -1176,7 +1176,8 @@ proc mips-dg-options { args } {
> > > || [mips_have_test_option_p options "-mpaired-
> > > single"]
> > > || [mips_have_test_option_p options "-
> > > mnan=legacy"]
> > > || [mips_have_test_option_p options "-
> > > mabs=legacy"]
> > > - || [mips_have_test_option_p options "!HAS_LSA"])
> > > } {
> > > + || [mips_have_test_option_p options "!HAS_LSA"]
> > > + || [mips_have_test_option_p options "-mbranch-
> > > likely"]) } {
> > > if { $gp_size == 32 } {
> > > mips_make_test_option options "-mips32r5"
> > > } else {
> >
> > Please can you make sure to retain the original patch formatting when
> > posting. I suspect you have copied this out of a putty session or similar
> > and
> > have therefore lost the tabs.
> >
> > The extra change is that in the post-arch option processing we will need to
> > infer -mno-branch-likely for the $isa_rev > 5 case much like we infer -
> > mnan=2008 and -mabs=2008. This is so that when running the testsuite using
> > -mips32r5 or earlier, with -mbranch-likely as part of the user-supplied test
> > flags, then a test which is upgraded to
> > mips32r6 does not attempt to use -mbranch-likely.
> >
> > Hope that wasn't too cryptic!
> >
> > Thanks,
> > Matthew
>
> The updated patch below includes the improved ChangeLog comment, correct
> formatting, and the post-arch enforcing of -mno-branch-likely for R6.
>
> Regards,
> Toma
>
> gcc/testsuite/ChangeLog:
>
> * gcc.target/mips/mips.exp (mips-dg-options): Downgrade to R5
> for -mbranch-likely and infer -mno-branch-likely for R6.
>
> diff --git a/gcc/testsuite/gcc.target/mips/mips.exp
> b/gcc/testsuite/gcc.target/mips/mips.exp
> index 7c24140..6b7c46f 100644
> --- a/gcc/testsuite/gcc.target/mips/mips.exp
> +++ b/gcc/testsuite/gcc.target/mips/mips.exp
> @@ -1176,7 +1176,8 @@ proc mips-dg-options { args } {
> || [mips_have_test_option_p options "-mpaired-single"]
> || [mips_have_test_option_p options "-mnan=legacy"]
> || [mips_have_test_option_p options "-mabs=legacy"]
> - || [mips_have_test_option_p options "!HAS_LSA"]) } {
> + || [mips_have_test_option_p options "!HAS_LSA"])
> + || [mips_have_test_option_p options "-mbranch-likely"])
> } {
> if { $gp_size == 32 } {
> mips_make_test_option options "-mips32r5"
> } else {
> @@ -1345,6 +1346,7 @@ proc mips-dg-options { args } {
> mips_make_test_option options "-mno-paired-single"
> mips_make_test_option options "-mnan=2008"
> mips_make_test_option options "-mabs=2008"
> + mips_make_test_option options "-mno-branch-likely"
> }
> if { [regexp {^-march=(octeon|loongson)} $arch] } {
> mips_make_test_option options "-mno-micromips"
An extra closing parenthesis sneaked in the previous version.
Removed in the patch below. Sorry about that.
Regards,
Toma
gcc/testsuite/ChangeLog:
* gcc.target/mips/mips.exp (mips-dg-options): Downgrade to R5
for -mbranch-likely and infer -mno-branch-likely for R6.
diff --git a/gcc/testsuite/gcc.target/mips/mips.exp
b/gcc/testsuite/gcc.target/mips/mips.exp
index 7c24140..39f44ff 100644
--- a/gcc/testsuite/gcc.target/mips/mips.exp
+++ b/gcc/testsuite/gcc.target/mips/mips.exp
@@ -1176,7 +1176,8 @@ proc mips-dg-options { args } {
|| [mips_have_test_option_p options "-mpaired-single"]
|| [mips_have_test_option_p options "-mnan=legacy"]
|| [mips_have_test_option_p options "-mabs=legacy"]
- || [mips_have_test_option_p options "!HAS_LSA"]) } {
+ || [mips_have_test_option_p options "!HAS_LSA"]
+ || [mips_have_test_option_p options "-mbranch-likely"])
} {
if { $gp_size == 32 } {
mips_make_test_option options "-mips32r5"
} else {
@@ -1345,6 +1346,7 @@ proc mips-dg-options { args } {
mips_make_test_option options "-mno-paired-single"
mips_make_test_option options "-mnan=2008"
mips_make_test_option options "-mabs=2008"
+ mips_make_test_option options "-mno-branch-likely"
}
if { [regexp {^-march=(octeon|loongson)} $arch] } {
mips_make_test_option options "-mno-micromips"
