Re: [PATCH 13/14][ARM/AArch64 testsuite] Use gcc-dg-runtest in advsimd-intrinsics.exp
On 22 April 2015 at 19:36, Alan Lawrence wrote: > In the first revision of Christophe Lyon's advsimd-intrinsics tests, > https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00532.html , both > gcc-dg-runtest (to assemble only) and c-torture-execute were used. In review > the gcc-dg-runtest part was then dropped, and execution tests continued > using c-torture-execute. However, c-torture-execute ignores e.g. dg-options > directives in the individual test files, whereas gcc-dg-runtest does not. > > This patch switches to gcc-dg-runtest (with dg-do-what-default = "run") for > all tests, allowing use of e.g. dg-options (in testsuite patch 3/3). This Sandra has recently committed a slightly different patch. If you want to update your, here are few comments/questions: - why do you add "-w" to additional_flags? - you changed the way we iterate over the tests, but this removes the possiblity to actually execute only a subset of the available tests, such as RUNTESTFLAGS=advsimd-intrinsics.exp=vadd.c Christophe. > generally seems to work OK - indeed I also dropped the > parallelization-disabling code - and the new advsimd-intrinsics.exp now > follows gcc.c-torture/compile/compile.exp and > gcc.c-torture/execute/execute.exp very closely. However, there are side > effects, of which we should be aware, but with which I think we can live, > specifically: > > (1) the lines in the test log change from... > > PASS: gcc.target/aarch64/advsimd-intrinsics/vcombine.c compilation, -O1 > PASS: gcc.target/aarch64/advsimd-intrinsics/vcombine.c execution, -O1 > > ...to... > > PASS: gcc.target/aarch64/advsimd-intrinsics/vcombine.c -O1 execution test > > (that is, the compilation line disappears, but the (test for excess errors) > remains unchanged) > > (2) The "-Og -g" variant is no longer tested; all of -O0, -O1, -O2, -O2 > -flto -fno-use-linker-plugin -flto-partition=none, -O2 -flto > -fuse-linker-plugin -fno-fat-lto-objects, -O3 -fomit-frame-pointer, -O3 -g, > -Os still are. My feeling is that this set of options is exhaustive enough. > > Cross-tested arm-none-eabi, aarch64-none-elf, aarch64_be-none-elf; natively > tested arm-none-linux-gnueabihf and aarch64-none-linux-gnu. > > gcc/testsuite/ChangeLog: > > * gcc.target/aarch64/advsimd-intrinsics/advsimd-intrinsics.exp: > Use gcc-dg-runtest.
Re: [PATCH 13/14][ARM/AArch64 testsuite] Use gcc-dg-runtest in advsimd-intrinsics.exp
Christophe Lyon wrote: On 22 April 2015 at 19:36, Alan Lawrence wrote: In the first revision of Christophe Lyon's advsimd-intrinsics tests, https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00532.html , both gcc-dg-runtest (to assemble only) and c-torture-execute were used. In review the gcc-dg-runtest part was then dropped, and execution tests continued using c-torture-execute. However, c-torture-execute ignores e.g. dg-options directives in the individual test files, whereas gcc-dg-runtest does not. This patch switches to gcc-dg-runtest (with dg-do-what-default = "run") for all tests, allowing use of e.g. dg-options (in testsuite patch 3/3). This Sandra has recently committed a slightly different patch. If you want to update your, here are few comments/questions: - why do you add "-w" to additional_flags? Hmmm. Not sure now. I agree, it appears to work without, so will drop that. - you changed the way we iterate over the tests, but this removes the possiblity to actually execute only a subset of the available tests, such as RUNTESTFLAGS=advsimd-intrinsics.exp=vadd.c I don't see this symptom - I am able to execute such subsets with either my, or Sandra's, advsimd-intrinsics.exp. Is it that you have to check runtest_file_p because you are setting gcc_parallel_test_enable to 0? I'm doing more testing now, but I think I can drop my advsimd-intrinsics.exp changes altogether; I'll post an updated patch series shortly. In the meantime I'm curious as to why you found the gcc_parallel_test_enable necessary? (And is it safe to reset it to 1 afterwards, rather than to a saved value?) TYVM for your other comments and review - I will incorporate all into my next revision. Thanks, Alan
Re: [PATCH 13/14][ARM/AArch64 testsuite] Use gcc-dg-runtest in advsimd-intrinsics.exp
On 26 May 2015 at 18:25, Alan Lawrence wrote: > Christophe Lyon wrote: >> >> On 22 April 2015 at 19:36, Alan Lawrence wrote: >>> >>> In the first revision of Christophe Lyon's advsimd-intrinsics tests, >>> https://gcc.gnu.org/ml/gcc-patches/2014-06/msg00532.html , both >>> gcc-dg-runtest (to assemble only) and c-torture-execute were used. In >>> review >>> the gcc-dg-runtest part was then dropped, and execution tests continued >>> using c-torture-execute. However, c-torture-execute ignores e.g. >>> dg-options >>> directives in the individual test files, whereas gcc-dg-runtest does not. >>> >>> This patch switches to gcc-dg-runtest (with dg-do-what-default = "run") >>> for >>> all tests, allowing use of e.g. dg-options (in testsuite patch 3/3). This >> >> >> Sandra has recently committed a slightly different patch. >> >> If you want to update your, here are few comments/questions: >> - why do you add "-w" to additional_flags? > > > Hmmm. Not sure now. I agree, it appears to work without, so will drop that. > >> - you changed the way we iterate over the tests, but this removes the >> possiblity to actually execute only a subset of the available tests, >> such as RUNTESTFLAGS=advsimd-intrinsics.exp=vadd.c > > > I don't see this symptom - I am able to execute such subsets with either my, > or Sandra's, advsimd-intrinsics.exp. I didn't try to run with your patch, I thought it was an oversight of yours. Sorry, indeed I've just checked that gcc-dg-runtest includes the filter. > > Is it that you have to check runtest_file_p because you are setting > gcc_parallel_test_enable to 0? > > I'm doing more testing now, but I think I can drop my advsimd-intrinsics.exp > changes altogether; I'll post an updated patch series shortly. > > In the meantime I'm curious as to why you found the gcc_parallel_test_enable > necessary? (And is it safe to reset it to 1 afterwards, rather than to a > saved value?) See https://gcc.gnu.org/ml/gcc/2014-10/msg00081.html > > TYVM for your other comments and review - I will incorporate all into my > next revision. Thanks. > > Thanks, Alan >
Re: [PATCH 13/14][ARM/AArch64 testsuite] Use gcc-dg-runtest in advsimd-intrinsics.exp
Christophe Lyon wrote:
On 26 May 2015 at 18:25, Alan Lawrence wrote:
I don't see this symptom - I am able to execute such subsets with either my,
or Sandra's, advsimd-intrinsics.exp.
I didn't try to run with your patch, I thought it was an oversight of yours.
Sorry, indeed I've just checked that gcc-dg-runtest includes the filter.
Is it that you have to check runtest_file_p because you are setting
gcc_parallel_test_enable to 0?
I'm doing more testing now, but I think I can drop my advsimd-intrinsics.exp
changes altogether; I'll post an updated patch series shortly.
In the meantime I'm curious as to why you found the gcc_parallel_test_enable
necessary? (And is it safe to reset it to 1 afterwards, rather than to a
saved value?)
See https://gcc.gnu.org/ml/gcc/2014-10/msg00081.html
So after working through the differences between Sandra's and my patch, I find
the existing advsimd-intrinsics.exp achieves pretty much the same thing, and
preserves the same list of test variants (e.g. the -Og -g from
set-torture-options which I had removed).
However, I've tried testing advsimd-intrinsics.exp (both the whole thing, and
individual tests using RUNTESTFLAGS) with and without this hunk:
@@ -57,20 +57,7 @@ set-torture-options $C_TORTURE_OPTIONS {{}} $LTO_TORTURE_OPTI
set additional_flags [add_options_for_arm_neon ""]
# Main loop.
-foreach src [lsort [glob -nocomplain $srcdir/$subdir/*.c]] {
-# If we're only testing specific files and this isn't one of them, skip it.
-if ![runtest_file_p $runtests $src] then {
- continue
-}
-
-# runtest_file_p is already run above, and the code below can run
-# runtest_file_p again, make sure everything for this test is
-# performed if the above runtest_file_p decided this runtest
-# instance should execute the test
-gcc_parallel_test_enable 0
-gcc-dg-runtest $src "" $additional_flags
-gcc_parallel_test_enable 1
-}
+gcc-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cS\]]] ""
${additional_flags}
and find exactly the same tests are run and pass. My hypothesis is thus that you
only need the explicit loop, manual checking of runtest_file_p, and
gcc_parallel_test_enable, in order to do *both* c-torture-execute *and*
gcc-dg-runtest; since we are now only doing the latter, this is unnecessary.
Does that make sense? (If you agree, I'll propose that as a standalone cleanup
patch.)
Cheers, Alan
Re: [PATCH 13/14][ARM/AArch64 testsuite] Use gcc-dg-runtest in advsimd-intrinsics.exp
On 28 May 2015 at 12:22, Alan Lawrence wrote:
> Christophe Lyon wrote:
>>
>> On 26 May 2015 at 18:25, Alan Lawrence wrote:
>>>
>>> I don't see this symptom - I am able to execute such subsets with either
>>> my,
>>> or Sandra's, advsimd-intrinsics.exp.
>>
>>
>> I didn't try to run with your patch, I thought it was an oversight of
>> yours.
>>
>> Sorry, indeed I've just checked that gcc-dg-runtest includes the filter.
>>
>>> Is it that you have to check runtest_file_p because you are setting
>>> gcc_parallel_test_enable to 0?
>>>
>>> I'm doing more testing now, but I think I can drop my
>>> advsimd-intrinsics.exp
>>> changes altogether; I'll post an updated patch series shortly.
>>>
>>> In the meantime I'm curious as to why you found the
>>> gcc_parallel_test_enable
>>> necessary? (And is it safe to reset it to 1 afterwards, rather than to a
>>> saved value?)
>>
>> See https://gcc.gnu.org/ml/gcc/2014-10/msg00081.html
>
>
> So after working through the differences between Sandra's and my patch, I
> find the existing advsimd-intrinsics.exp achieves pretty much the same
> thing, and preserves the same list of test variants (e.g. the -Og -g from
> set-torture-options which I had removed).
>
> However, I've tried testing advsimd-intrinsics.exp (both the whole thing,
> and individual tests using RUNTESTFLAGS) with and without this hunk:
>
> @@ -57,20 +57,7 @@ set-torture-options $C_TORTURE_OPTIONS {{}}
> $LTO_TORTURE_OPTI
> set additional_flags [add_options_for_arm_neon ""]
>
> # Main loop.
> -foreach src [lsort [glob -nocomplain $srcdir/$subdir/*.c]] {
> -# If we're only testing specific files and this isn't one of them, skip
> it.
> -if ![runtest_file_p $runtests $src] then {
> - continue
> -}
> -
> -# runtest_file_p is already run above, and the code below can run
> -# runtest_file_p again, make sure everything for this test is
> -# performed if the above runtest_file_p decided this runtest
> -# instance should execute the test
> -gcc_parallel_test_enable 0
> -gcc-dg-runtest $src "" $additional_flags
> -gcc_parallel_test_enable 1
> -}
> +gcc-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cS\]]] ""
> ${additional_flags}
>
> and find exactly the same tests are run and pass. My hypothesis is thus that
> you only need the explicit loop, manual checking of runtest_file_p, and
> gcc_parallel_test_enable, in order to do *both* c-torture-execute *and*
> gcc-dg-runtest; since we are now only doing the latter, this is unnecessary.
> Does that make sense? (If you agree, I'll propose that as a standalone
> cleanup patch.)
>
Indeed I think you are right. Since we no longer call
c-torture-execute, we no longer need to call runtest_file_p here.
Having only one remaining call to runtest_file_p in gcc-dg-runtest is
parallel-safe. Thanks for the cleanup.
Christophe.
> Cheers, Alan
>
Re: [PATCH 13/14][ARM/AArch64 testsuite] Use gcc-dg-runtest in advsimd-intrinsics.exp
On 28 May 2015 at 13:32, Christophe Lyon wrote:
> On 28 May 2015 at 12:22, Alan Lawrence wrote:
>> Christophe Lyon wrote:
>>>
>>> On 26 May 2015 at 18:25, Alan Lawrence wrote:
I don't see this symptom - I am able to execute such subsets with either
my,
or Sandra's, advsimd-intrinsics.exp.
>>>
>>>
>>> I didn't try to run with your patch, I thought it was an oversight of
>>> yours.
>>>
>>> Sorry, indeed I've just checked that gcc-dg-runtest includes the filter.
>>>
Is it that you have to check runtest_file_p because you are setting
gcc_parallel_test_enable to 0?
I'm doing more testing now, but I think I can drop my
advsimd-intrinsics.exp
changes altogether; I'll post an updated patch series shortly.
In the meantime I'm curious as to why you found the
gcc_parallel_test_enable
necessary? (And is it safe to reset it to 1 afterwards, rather than to a
saved value?)
>>>
>>> See https://gcc.gnu.org/ml/gcc/2014-10/msg00081.html
>>
>>
>> So after working through the differences between Sandra's and my patch, I
>> find the existing advsimd-intrinsics.exp achieves pretty much the same
>> thing, and preserves the same list of test variants (e.g. the -Og -g from
>> set-torture-options which I had removed).
>>
>> However, I've tried testing advsimd-intrinsics.exp (both the whole thing,
>> and individual tests using RUNTESTFLAGS) with and without this hunk:
>>
>> @@ -57,20 +57,7 @@ set-torture-options $C_TORTURE_OPTIONS {{}}
>> $LTO_TORTURE_OPTI
>> set additional_flags [add_options_for_arm_neon ""]
>>
>> # Main loop.
>> -foreach src [lsort [glob -nocomplain $srcdir/$subdir/*.c]] {
>> -# If we're only testing specific files and this isn't one of them, skip
>> it.
>> -if ![runtest_file_p $runtests $src] then {
>> - continue
>> -}
>> -
>> -# runtest_file_p is already run above, and the code below can run
>> -# runtest_file_p again, make sure everything for this test is
>> -# performed if the above runtest_file_p decided this runtest
>> -# instance should execute the test
>> -gcc_parallel_test_enable 0
>> -gcc-dg-runtest $src "" $additional_flags
>> -gcc_parallel_test_enable 1
>> -}
>> +gcc-dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cS\]]] ""
>> ${additional_flags}
>>
>> and find exactly the same tests are run and pass. My hypothesis is thus that
>> you only need the explicit loop, manual checking of runtest_file_p, and
>> gcc_parallel_test_enable, in order to do *both* c-torture-execute *and*
>> gcc-dg-runtest; since we are now only doing the latter, this is unnecessary.
>> Does that make sense? (If you agree, I'll propose that as a standalone
>> cleanup patch.)
>>
>
> Indeed I think you are right. Since we no longer call
> c-torture-execute, we no longer need to call runtest_file_p here.
> Having only one remaining call to runtest_file_p in gcc-dg-runtest is
> parallel-safe. Thanks for the cleanup.
>
So in fact, except for the comment about '-w' it seems you initial
patch was mostly OK, right?
> Christophe.
>
>> Cheers, Alan
>>
Re: [PATCH 13/14][ARM/AArch64 testsuite] Use gcc-dg-runtest in advsimd-intrinsics.exp
Christophe Lyon wrote: So in fact, except for the comment about '-w' it seems you initial patch was mostly OK, right? Well, my removing a bunch of that c-torture-init stuff, was what was causing the "-Og -g" variant to go missing, but apart from that, yes. --Alan
