RE: [PATCH] MIPS: Correctly update the isa and arch_test_option_p variables after the arch dependency handling code in mips.exp
Ok to commit? Yes, this is OK. Committed as SVN 225813. Regards, Andrew
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?
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-r1
RE: [PATCH] MIPS: Correctly update the isa and arch_test_option_p variables after the arch dependency handling code in mips.exp
-Original Message-
From: Andrew Bennett [mailto:andrew.benn...@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
Matthew Fortune matthew.fort...@imgtec.com writes:
Andrew Bennett andrew.benn...@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
[PATCH] MIPS: Correctly update the isa and arch_test_option_p variables after the arch dependency handling code in mips.exp
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
Andrew Bennett andrew.benn...@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
