| Message ID | B5E67142681B53468FAF6B7C313565624F505B1D@HHMAIL01.hh.imgtec.org |
|---|---|
| State | New |
| Headers | show |
On 08/23/2016 02:15 AM, Robert Suchanek wrote: > Hi, > >> unfortunately this broke make check-c >> RUNTESTFLAGS='vect.exp=*no-vfa-vect-dv-2.c >> --target_board=unix\{-m32,-m64\}', causing the check if >> vect_aligned_arrays to be cached between the -m64 and -m32 variants >> which is incorrect at least on my machine if you actually run that test >> for -m32 and -m64 you get different results. In both case et_index is 0 >> so you use the cached value the second time, but that's not correct >> because the options changed. >> >> I suspect this also causes some random vectorizer tests to appear and >> disappear during regression testing with the same -m64 and -m32, but I'm >> not absolutely sure of that part. >> >> Thanks! >> >> Trev > > I was misled by the comments in a few procedures suggesting that the results > should have been cached and the use of global variable looked mistyped. Comments wrong, surely you jest :-) > > The following patch reverts to the old behaviour. I also removed misleading > comments and related logic that checks for the cached result. There might be > other procedures with similar inconsistency but here I only modified the offending ones. Thanks. Given how much cut-n-paste we do with the tcl code there's a good chance this problem exists elsewhere. I don't start ranting about tcl/dejagnu, I'll just put me in a terrible mood for the rest of the day. > > Alternatively, it would be possible to switch to the new method and do the caching > but it is more intrusive change that requires careful analysis of the results > and the tests will not likely be directly comparable with old results (because of > flag mixing into the test names). It's safer to restore the original behaviour > as the patch was not supposed to change any existing results. I'd go with the safer approach for now -- I keep holding out hope that there's a silver bullet out there that will allow us to replace dejagnu and all its tcl goop some day. > > Ok to apply? > > Regards, > Robert > > gcc/testsuite/ > * lib/target-supports.exp > (check_effective_target_vect_aligned_arrays): Don't cache the result. > (check_effective_target_vect_natural_alignment): Ditto. > (check_effective_target_vector_alignment_reachable): Ditto. > (check_effective_target_vector_alignment_reachable_for_64bit): Ditto. OK. jeff >
On Tue, Aug 23, 2016 at 09:52:23AM -0600, Jeff Law wrote: > On 08/23/2016 02:15 AM, Robert Suchanek wrote: > > Hi, > > > > > unfortunately this broke make check-c > > > RUNTESTFLAGS='vect.exp=*no-vfa-vect-dv-2.c > > > --target_board=unix\{-m32,-m64\}', causing the check if > > > vect_aligned_arrays to be cached between the -m64 and -m32 variants > > > which is incorrect at least on my machine if you actually run that test > > > for -m32 and -m64 you get different results. In both case et_index is 0 > > > so you use the cached value the second time, but that's not correct > > > because the options changed. > > > > > > I suspect this also causes some random vectorizer tests to appear and > > > disappear during regression testing with the same -m64 and -m32, but I'm > > > not absolutely sure of that part. > > > > > > Thanks! > > > > > > Trev > > > > I was misled by the comments in a few procedures suggesting that the results > > should have been cached and the use of global variable looked mistyped. > Comments wrong, surely you jest :-) > > > > > The following patch reverts to the old behaviour. I also removed misleading > > comments and related logic that checks for the cached result. There might be > > other procedures with similar inconsistency but here I only modified the offending ones. > Thanks. Given how much cut-n-paste we do with the tcl code there's a good > chance this problem exists elsewhere. I don't start ranting about > tcl/dejagnu, I'll just put me in a terrible mood for the rest of the day. > > > > > Alternatively, it would be possible to switch to the new method and do the caching > > but it is more intrusive change that requires careful analysis of the results > > and the tests will not likely be directly comparable with old results (because of > > flag mixing into the test names). It's safer to restore the original behaviour > > as the patch was not supposed to change any existing results. > I'd go with the safer approach for now -- I keep holding out hope that > there's a silver bullet out there that will allow us to replace dejagnu and > all its tcl goop some day. I've certainly been tempted to take a stab at at least replacing the expect stuff with something else, it drives me kind of crazy to see how much testsuite time is spent running expect. Even if we can't do all of it, the vast majority is just run the compiler and grep dump files or the compiler's stdout / stderr. Trev
On 08/23/2016 04:15 PM, Trevor Saunders wrote: > > I've certainly been tempted to take a stab at at least replacing the > expect stuff with something else, it drives me kind of crazy to see how > much testsuite time is spent running expect. Even if we can't do all of > it, the vast majority is just run the compiler and grep dump files or > the compiler's stdout / stderr. Yup. When Rob was making noise about rewriting dejagnu I tried to steer him a bit towards instead replacing the expect layer, but his plans where more on the tcl/dejagnu side. I don't think anything ever came out of that proposal. Expect is severe overkill for what we need and yes, it burns an amazing about of CPU time for what get out of it. Jeff
> On 08/23/2016 04:15 PM, Trevor Saunders wrote: > > > > I've certainly been tempted to take a stab at at least replacing the > > expect stuff with something else, it drives me kind of crazy to see how > > much testsuite time is spent running expect. Even if we can't do all of > > it, the vast majority is just run the compiler and grep dump files or > > the compiler's stdout / stderr. > Yup. When Rob was making noise about rewriting dejagnu I tried to steer > him a bit towards instead replacing the expect layer, but his plans > where more on the tcl/dejagnu side. I don't think anything ever came > out of that proposal. > > Expect is severe overkill for what we need and yes, it burns an amazing > about of CPU time for what get out of it. > > Jeff True. I wasn't considering replacing the expect layer, just to overcome the limitation of auto-vect tests. This in itself is rather another hack but didn't have a suggestion what to replace it with. Regards, Robert
Hi Jeff, > > The following patch reverts to the old behaviour. I also removed misleading > > comments and related logic that checks for the cached result. There might be > > other procedures with similar inconsistency but here I only modified the > offending ones. > Thanks. Given how much cut-n-paste we do with the tcl code there's a > good chance this problem exists elsewhere. I don't start ranting about > tcl/dejagnu, I'll just put me in a terrible mood for the rest of the day. Indeed. > > Alternatively, it would be possible to switch to the new method and do the > caching > > but it is more intrusive change that requires careful analysis of the results > > and the tests will not likely be directly comparable with old results > (because of > > flag mixing into the test names). It's safer to restore the original > behaviour > > as the patch was not supposed to change any existing results. > I'd go with the safer approach for now -- I keep holding out hope that > there's a silver bullet out there that will allow us to replace dejagnu > and all its tcl goop some day. I noticed over time that tcl/dejagnu have a unique set of features. Replacing them with something else at some point wouldn't be a bad thing. > > > > > Ok to apply? > > > > Regards, > > Robert > > > > gcc/testsuite/ > > * lib/target-supports.exp > > (check_effective_target_vect_aligned_arrays): Don't cache the result. > > (check_effective_target_vect_natural_alignment): Ditto. > > (check_effective_target_vector_alignment_reachable): Ditto. > > (check_effective_target_vector_alignment_reachable_for_64bit): Ditto. > OK. > jeff > > Committed as r239730. Regards, Robert
diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp index 533d3a6..0dabea0 100644 --- a/gcc/testsuite/lib/target-supports.exp +++ b/gcc/testsuite/lib/target-supports.exp @@ -5309,32 +5309,22 @@ proc check_effective_target_vect_hw_misalign { } { # Return 1 if arrays are aligned to the vector alignment # boundary, 0 otherwise. -# -# This won't change for different subtargets so cache the result. proc check_effective_target_vect_aligned_arrays { } { - global et_vect_aligned_arrays_saved - global et_index - - if [info exists et_vect_aligned_arrays_saved($et_index)] { - verbose "check_effective_target_vect_aligned_arrays:\ - using cached result" 2 - } else { - set et_vect_aligned_arrays_saved($et_index) 0 - if { ([istarget x86_64-*-*] || [istarget i?86-*-*]) } { - if { ([is-effective-target lp64] - && ( ![check_avx_available] - || [check_prefer_avx128])) } { - set et_vect_aligned_arrays_saved($et_index) 1 - } - } - if [istarget spu-*-*] { - set et_vect_aligned_arrays_saved($et_index) 1 + set et_vect_aligned_arrays 0 + if { ([istarget x86_64-*-*] || [istarget i?86-*-*]) } { + if { ([is-effective-target lp64] + && ( ![check_avx_available] + || [check_prefer_avx128])) } { + set et_vect_aligned_arrays 1 } } + if [istarget spu-*-*] { + set et_vect_aligned_arrays 1 + } verbose "check_effective_target_vect_aligned_arrays:\ - returning $et_vect_aligned_arrays_saved($et_index)" 2 - return $et_vect_aligned_arrays_saved($et_index) + returning $et_vect_aligned_arrays" 2 + return $et_vect_aligned_arrays } # Return 1 if types of size 32 bit or less are naturally aligned @@ -5382,74 +5372,43 @@ proc check_effective_target_natural_alignment_64 { } { # Return 1 if all vector types are naturally aligned (aligned to their # type-size), 0 otherwise. -# -# This won't change for different subtargets so cache the result. proc check_effective_target_vect_natural_alignment { } { - global et_vect_natural_alignment_saved - global et_index - - if [info exists et_vect_natural_alignment_saved($et_index)] { - verbose "check_effective_target_vect_natural_alignment: using cached result" 2 - } else { - set et_vect_natural_alignment_saved($et_index) 1 - if { [check_effective_target_arm_eabi] - || [istarget nvptx-*-*] - || [istarget s390*-*-*] } { - set et_vect_natural_alignment_saved($et_index) 0 - } + set et_vect_natural_alignment 1 + if { [check_effective_target_arm_eabi] + || [istarget nvptx-*-*] + || [istarget s390*-*-*] } { + set et_vect_natural_alignment 0 } verbose "check_effective_target_vect_natural_alignment:\ - returning $et_vect_natural_alignment_saved($et_index)" 2 - return $et_vect_natural_alignment_saved($et_index) + returning $et_vect_natural_alignment" 2 + return $et_vect_natural_alignment } # Return 1 if vector alignment (for types of size 32 bit or less) is reachable, 0 otherwise. -# -# This won't change for different subtargets so cache the result. proc check_effective_target_vector_alignment_reachable { } { - global et_vector_alignment_reachable_saved - global et_index - - if [info exists et_vector_alignment_reachable_saved($et_index)] { - verbose "check_effective_target_vector_alignment_reachable:\ - using cached result" 2 - } else { - if { [check_effective_target_vect_aligned_arrays] - || [check_effective_target_natural_alignment_32] } { - set et_vector_alignment_reachable_saved($et_index) 1 - } else { - set et_vector_alignment_reachable_saved($et_index) 0 - } + set et_vector_alignment_reachable 0 + if { [check_effective_target_vect_aligned_arrays] + || [check_effective_target_natural_alignment_32] } { + set et_vector_alignment_reachable 1 } verbose "check_effective_target_vector_alignment_reachable:\ - returning $et_vector_alignment_reachable_saved($et_index)" 2 - return $et_vector_alignment_reachable_saved($et_index) + returning $et_vector_alignment_reachable" 2 + return $et_vector_alignment_reachable } # Return 1 if vector alignment for 64 bit is reachable, 0 otherwise. -# -# This won't change for different subtargets so cache the result. proc check_effective_target_vector_alignment_reachable_for_64bit { } { - global et_vector_alignment_reachable_for_64bit_saved - global et_index - - if [info exists et_vector_alignment_reachable_for_64bit_saved($et_index)] { - verbose "check_effective_target_vector_alignment_reachable_for_64bit:\ - using cached result" 2 - } else { - if { [check_effective_target_vect_aligned_arrays] - || [check_effective_target_natural_alignment_64] } { - set et_vector_alignment_reachable_for_64bit_saved($et_index) 1 - } else { - set et_vector_alignment_reachable_for_64bit_saved($et_index) 0 - } + set et_vector_alignment_reachable_for_64bit 0 + if { [check_effective_target_vect_aligned_arrays] + || [check_effective_target_natural_alignment_64] } { + set et_vector_alignment_reachable_for_64bit 1 } verbose "check_effective_target_vector_alignment_reachable_for_64bit:\ - returning $et_vector_alignment_reachable_for_64bit_saved($et_index)" 2 - return $et_vector_alignment_reachable_for_64bit_saved($et_index) + returning $et_vector_alignment_reachable_for_64bit" 2 + return $et_vector_alignment_reachable_for_64bit } # Return 1 if the target only requires element alignment for vector accesses