Message ID | A614194ED15B4844BC4C9FB7F21FCD9222530791@HHMAIL01.hh.imgtec.org |
---|---|
State | New |
Headers | show |
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
> 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
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 --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"