diff mbox

[testsuite] MIPS: Downgrade R6 to R5 if tests need branch-likely instructions.

Message ID A614194ED15B4844BC4C9FB7F21FCD9222530791@HHMAIL01.hh.imgtec.org
State New
Headers show

Commit Message

Toma Tabacu Nov. 4, 2016, 4:17 p.m. UTC 
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Toma Tabacu
> Sent: 04 November 2016 15:25
> To: Matthew Fortune; gcc-patches@gcc.gnu.org
> Cc: catherine_moore@mentor.com
> 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; gcc-patches@gcc.gnu.org
> > Cc: catherine_moore@mentor.com
> > Subject: RE: [PATCH,testsuite] MIPS: Downgrade R6 to R5 if tests need
> > branch-likely instructions.
> >
> > Toma Tabacu <Toma.Tabacu@imgtec.com> 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.

Comments

Matthew Fortune Nov. 4, 2016, 4:48 p.m. UTC | #1 
Toma Tabacu <Toma.Tabacu@imgtec.com> writes:
> > From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> > owner@gcc.gnu.org] On Behalf Of Toma Tabacu
> > Sent: 04 November 2016 15:25
> > To: Matthew Fortune; gcc-patches@gcc.gnu.org
> > Cc: catherine_moore@mentor.com
> > 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; gcc-patches@gcc.gnu.org
> > > Cc: catherine_moore@mentor.com
> > > Subject: RE: [PATCH,testsuite] MIPS: Downgrade R6 to R5 if tests
> > > need branch-likely instructions.
> > >
> > > Toma Tabacu <Toma.Tabacu@imgtec.com> 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.

Thanks, committed as r241850.

Matthew
Toma Tabacu Nov. 4, 2016, 5:08 p.m. UTC | #2 
> From: Matthew Fortune
> Sent: 04 November 2016 16:49
> To: Toma Tabacu; gcc-patches@gcc.gnu.org
> Cc: catherine_moore@mentor.com
> Subject: RE: [PATCH,testsuite] MIPS: Downgrade R6 to R5 if tests need branch-
> likely instructions.
> 
> Toma Tabacu <Toma.Tabacu@imgtec.com> writes:
> > > From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> > > owner@gcc.gnu.org] On Behalf Of Toma Tabacu
> > > Sent: 04 November 2016 15:25
> > > To: Matthew Fortune; gcc-patches@gcc.gnu.org
> > > Cc: catherine_moore@mentor.com
> > > 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; gcc-patches@gcc.gnu.org
> > > > Cc: catherine_moore@mentor.com
> > > > Subject: RE: [PATCH,testsuite] MIPS: Downgrade R6 to R5 if tests
> > > > need branch-likely instructions.
> > > >
> > > > Toma Tabacu <Toma.Tabacu@imgtec.com> 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.
> 
> Thanks, committed as r241850.
> 
> Matthew

I've noticed that there is a typo in my surname in the ChangeLog entry (in the
name and in the email address).

Regards,
Toma Tabacu
Matthew Fortune Nov. 4, 2016, 5:14 p.m. UTC | #3 
Toma Tabacu <Toma.Tabacu@imgtec.com> writes:
> I've noticed that there is a typo in my surname in the ChangeLog entry
> (in the name and in the email address).

Apologies, corrected.

Matthew
diff mbox

Patch

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"