diff mbox

MIPS: Correctly update the isa and arch_test_option_p variables after the arch dependency handling code in mips.exp

Message ID 0DA23CC379F5F945ACB41CF394B98277210F525A@LEMAIL01.le.imgtec.org
State New
Headers show

Commit Message

Andrew Bennett July 14, 2015, 1:23 p.m. UTC 
> Yeah, I agree that this doesn't really fit the model that well,
> but like you say, we're stretching the logic a bit :-).  When I wrote it,
> the architectures formed a nice tree in which moving to leaf nodes only
> added features.  So in the pre-r6 days:
> 
>     # Handle dependencies between the pre-arch options and the arch option.
>     # This should mirror the arch and post-arch code below.
>     if { !$arch_test_option_p } {
> 
> increased the architecture from the --target_board default to match
> the features required by the test, whereas:
> 
>     # Handle dependencies between the arch option and the post-arch options.
>     # This should mirror the arch and pre-arch code above.
>     if { $arch_test_option_p } {
> 
> turned off features from the --target_board default to match a lower
> architecture required by the test.  So in the pre-r6 days, all the code
> in the second block was turning something off when going to a lower
> architecture.  The blocks were mutually-exclusive and writing it this
> way reduced the number of redundant options.  (Admittedly you could argue
> that it's daft to worry about that given the kind of command lines you
> tend to get from the rest of mips.exp. :-))
> 
> r6 is the first time we've had to turn something off when moving up.
> -mnan and -mabs are also the first options where old architectures
> support only A, higher revisions support A and B, and the newest
> revision supports only B.  I think I'd prefer to acknowledge that
> and have:
> 
>     # Handle dependencies between the arch option and the post-arch options.
>     # This should mirror the arch and pre-arch code above.  For pre-r6
>     # architectures this only needs to be done when we've moved down
>     # to a lower architecture and might need to turn features off,
>     # but moving up from pre-r6 to r6 can remove features too.
>     if { $arch_test_option_p || ($orig_isa_rev < 6 && $isa_rev >= 6) } {
> 
> I think the existing r6->r5 case really is different: there we're
> forcing a -mnan option not because the architecture needs it but
> because the environment might.

Hi,

An updated patch and ChangeLog which reflects the comments is below.
I have tested it on the mips-img-elf and mips-mti-elf toolchains
with all the valid multilib configurations and there are no new regressions.

Ok to commit?


Regards,


Andrew



testsuite/
	* gcc.target/mips/mips.exp (mips-dg-options): Allow the post-arch
	code to be run when the pre-arch code increases the isa_rev to 
	mips32r6 or greater.

Comments

Moore, Catherine July 14, 2015, 9:26 p.m. UTC | #1 
> -----Original Message-----
> From: Andrew Bennett [mailto:Andrew.Bennett@imgtec.com]
> Sent: Tuesday, July 14, 2015 9:24 AM
> To: Richard Sandiford; Matthew Fortune
> Cc: gcc-patches@gcc.gnu.org; Moore, Catherine
> Subject: RE: [PATCH] MIPS: Correctly update the isa and arch_test_option_p
> variables after the arch dependency handling code in mips.exp
> 
> > Yeah, I agree that this doesn't really fit the model that well, but
> > like you say, we're stretching the logic a bit :-).  When I wrote it,
> > the architectures formed a nice tree in which moving to leaf nodes
> > only added features.  So in the pre-r6 days:
> >
> >     # Handle dependencies between the pre-arch options and the arch
> option.
> >     # This should mirror the arch and post-arch code below.
> >     if { !$arch_test_option_p } {
> >
> > increased the architecture from the --target_board default to match
> > the features required by the test, whereas:
> >
> >     # Handle dependencies between the arch option and the post-arch
> options.
> >     # This should mirror the arch and pre-arch code above.
> >     if { $arch_test_option_p } {
> >
> > turned off features from the --target_board default to match a lower
> > architecture required by the test.  So in the pre-r6 days, all the
> > code in the second block was turning something off when going to a
> > lower architecture.  The blocks were mutually-exclusive and writing it
> > this way reduced the number of redundant options.  (Admittedly you
> > could argue that it's daft to worry about that given the kind of
> > command lines you tend to get from the rest of mips.exp. :-))
> >
> > r6 is the first time we've had to turn something off when moving up.
> > -mnan and -mabs are also the first options where old architectures
> > support only A, higher revisions support A and B, and the newest
> > revision supports only B.  I think I'd prefer to acknowledge that and
> > have:
> >
> >     # Handle dependencies between the arch option and the post-arch
> options.
> >     # This should mirror the arch and pre-arch code above.  For pre-r6
> >     # architectures this only needs to be done when we've moved down
> >     # to a lower architecture and might need to turn features off,
> >     # but moving up from pre-r6 to r6 can remove features too.
> >     if { $arch_test_option_p || ($orig_isa_rev < 6 && $isa_rev >= 6) }
> > {
> >
> > I think the existing r6->r5 case really is different: there we're
> > forcing a -mnan option not because the architecture needs it but
> > because the environment might.
> 
> Hi,
> 
> An updated patch and ChangeLog which reflects the comments is below.
> I have tested it on the mips-img-elf and mips-mti-elf toolchains with all the
> valid multilib configurations and there are no new regressions.
> 
> Ok to commit?

Yes, this is OK.
> 
> 
> testsuite/
> 	* gcc.target/mips/mips.exp (mips-dg-options): Allow the post-arch
> 	code to be run when the pre-arch code increases the isa_rev to
> 	mips32r6 or greater.
> 
>
Andrew Bennett July 15, 2015, 9:26 a.m. UTC | #2 
> > Ok to commit?
> 
> Yes, this is OK.

Committed as SVN 225813.

Regards,


Andrew
diff mbox

Patch

diff --git a/gcc/testsuite/gcc.target/mips/mips.exp b/gcc/testsuite/gcc.target/mips/mips.exp
index 1dd4173..b3617e4 100644
--- a/gcc/testsuite/gcc.target/mips/mips.exp
+++ b/gcc/testsuite/gcc.target/mips/mips.exp
@@ -1045,6 +1045,7 @@  proc mips-dg-options { args } {
     set arch [mips_option options arch]
     set isa [mips_arch_info $arch isa]
     set isa_rev [mips_arch_info $arch isa_rev]
+    set orig_isa_rev $isa_rev
 
     # If the test forces a 32-bit architecture, force -mgp32.
     # Force the current -mgp setting otherwise; if we don't,
@@ -1279,8 +1280,11 @@  proc mips-dg-options { args } {
     }
 
     # Handle dependencies between the arch option and the post-arch options.
-    # This should mirror the arch and pre-arch code above.
-    if { $arch_test_option_p } {
+    # This should mirror the arch and pre-arch code above.  For pre-r6
+    # architectures this only needs to be done when we've moved down
+    # to a lower architecture and might need to turn features off,
+    # but moving up from pre-r6 to r6 can remove features too.
+    if { $arch_test_option_p || ($orig_isa_rev < 6 && $isa_rev >= 6) } {
        if { $isa < 2 } {
            mips_make_test_option options "-mno-branch-likely"
            mips_make_test_option options "-mno-fix-r10000"