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

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

[Andrew Bennett]

Hi,

I have noticed that in the mips.exp dg-option handling code the isa and 
arch_test_option_p variables are not updated after the pre-arch to arch 
dependency handling.  This means that if this code changes the 
architecture the post-arch dependency handling code (which relies on 
arch_test_option_p being true) is not run to handle any extra dependencies 
the new architecture might need.  

I have found this issue while investigating failures with the mips-mti-elf 
toolchain using the -mnan=legacy multilib flags when running any of the 
mips tests that have the HAS_LSA option specified in the dg-options.  The 
default architecture for this toolchain is mips32r2.  This means the architecture 
handling code changes the architecture to mips32r6 to handle the HAS_LSA 
requirements.  Unfortunately because the arch_test_option_p is not updated 
it is still set to false, so the post-arch code is not run.  This means
the nan encoding is not set to -mnan=2008 when then causes the tests to fail 
because mips32r6 does not support -mnan=legacy.	

The patch and ChangeLog are below.

Ok to commit?



Regards,



Andrew


testsuite/
	* gcc.target/mips/mips.exp (mips-dg-options): Update the isa and
	arch_test_option_p variables after the arch dependency handling code.


diff --git a/gcc/testsuite/gcc.target/mips/mips.exp b/gcc/testsuite/gcc.target/mips/mips.exp
index 1dd4173..1eb714d 100644
--- a/gcc/testsuite/gcc.target/mips/mips.exp
+++ b/gcc/testsuite/gcc.target/mips/mips.exp
@@ -1188,8 +1188,10 @@ proc mips-dg-options { args } {
     }
 
     # Re-calculate the isa_rev for use in the abi handling code below
+    set arch_test_option_p [mips_test_option_p options arch]
     set arch [mips_option options arch]
     set isa_rev [mips_arch_info $arch isa_rev]
+    set isa [mips_arch_info $arch isa]
 
     # Set an appropriate ABI, handling dependencies between the pre-abi
     # options and the abi options.  This should mirror the abi and post-abi

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

[Matthew Fortune]

Andrew Bennett <Andrew.Bennett@imgtec.com> writes:
> I have noticed that in the mips.exp dg-option handling code the isa and
> arch_test_option_p variables are not updated after the pre-arch to arch
> dependency handling.  This means that if this code changes the
> architecture the post-arch dependency handling code (which relies on
> arch_test_option_p being true) is not run to handle any extra dependencies
> the new architecture might need.

I'm not sure this is the right place to fix this, though it does seem
subjective as we are stretching the logic a little I think.

In the pre-arch options (i.e. when an arch is not explicitly requested) we
already have code that sets -mnan-2008 when downgrading a test R6 to R5 as
the R6 headers will be nan2008 and there is no guarantee of nan legacy headers
existing. This is the opposite case where we upgrade a test from R5 to R6
and R6 has to use -mnan=2008 so needs to explicitly override any command line
option to use -mnan=legacy. I think that therefore needs adding when we set
the arch to R6 in the pre-arch options.

At the same time I think we need to add -mabs=2008 in the same place as R6
requires ABS2008 as well. You should see that as a failure if you test with
-mabs=legacy.

I think I wrote the exact same patch as you have when I did the original R6
tests and concluded it was not in-keeping with the structure of mips.exp.

I've added Richard too since he may be able to offer a guiding hand as original
author of most of the mips.exp code.

Thanks,
Matthew

> I have found this issue while investigating failures with the mips-mti-elf
> toolchain using the -mnan=legacy multilib flags when running any of the
> mips tests that have the HAS_LSA option specified in the dg-options.  The
> default architecture for this toolchain is mips32r2.  This means the architecture
> handling code changes the architecture to mips32r6 to handle the HAS_LSA
> requirements.  Unfortunately because the arch_test_option_p is not updated
> it is still set to false, so the post-arch code is not run.  This means
> the nan encoding is not set to -mnan=2008 when then causes the tests to fail
> because mips32r6 does not support -mnan=legacy.
> 
> The patch and ChangeLog are below.
> 
> Ok to commit?
> 
> 
> 
> Regards,
> 
> 
> 
> Andrew
> 
> 
> testsuite/
> 	* gcc.target/mips/mips.exp (mips-dg-options): Update the isa and
> 	arch_test_option_p variables after the arch dependency handling code.
> 
> 
> diff --git a/gcc/testsuite/gcc.target/mips/mips.exp
> b/gcc/testsuite/gcc.target/mips/mips.exp
> index 1dd4173..1eb714d 100644
> --- a/gcc/testsuite/gcc.target/mips/mips.exp
> +++ b/gcc/testsuite/gcc.target/mips/mips.exp
> @@ -1188,8 +1188,10 @@ proc mips-dg-options { args } {
>      }
> 
>      # Re-calculate the isa_rev for use in the abi handling code below
> +    set arch_test_option_p [mips_test_option_p options arch]
>      set arch [mips_option options arch]
>      set isa_rev [mips_arch_info $arch isa_rev]
> +    set isa [mips_arch_info $arch isa]
> 
>      # Set an appropriate ABI, handling dependencies between the pre-abi
>      # options and the abi options.  This should mirror the abi and post-abi

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

[Richard Sandiford]

Matthew Fortune <Matthew.Fortune@imgtec.com> writes:
> Andrew Bennett <Andrew.Bennett@imgtec.com> writes:
>> I have noticed that in the mips.exp dg-option handling code the isa and
>> arch_test_option_p variables are not updated after the pre-arch to arch
>> dependency handling.  This means that if this code changes the
>> architecture the post-arch dependency handling code (which relies on
>> arch_test_option_p being true) is not run to handle any extra dependencies
>> the new architecture might need.
>
> I'm not sure this is the right place to fix this, though it does seem
> subjective as we are stretching the logic a little I think.
>
> In the pre-arch options (i.e. when an arch is not explicitly requested) we
> already have code that sets -mnan-2008 when downgrading a test R6 to R5 as
> the R6 headers will be nan2008 and there is no guarantee of nan legacy headers
> existing. This is the opposite case where we upgrade a test from R5 to R6
> and R6 has to use -mnan=2008 so needs to explicitly override any command line
> option to use -mnan=legacy. I think that therefore needs adding when we set
> the arch to R6 in the pre-arch options.
>
> At the same time I think we need to add -mabs=2008 in the same place as R6
> requires ABS2008 as well. You should see that as a failure if you test with
> -mabs=legacy.
>
> I think I wrote the exact same patch as you have when I did the original R6
> tests and concluded it was not in-keeping with the structure of mips.exp.
>
> I've added Richard too since he may be able to offer a guiding hand as
> original author of most of the mips.exp code.

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.

Thanks,
Richard

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

[Andrew Bennett]

> 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. 


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"

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

[Moore, Catherine]

> -----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.
> 
> 

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

[Andrew Bennett]

> > Ok to commit?
> 
> Yes, this is OK.

Committed as SVN 225813.

Regards,


Andrew