diff mbox series

[committed] RISC-V: Make stack_save_restore tests more robust

Message ID 10382874-cea3-93d3-de15-bc3b7383cd61@ventanamicro.com
State New
Headers show
Series [committed] RISC-V: Make stack_save_restore tests more robust | expand

Commit Message

Jeff Law Aug. 25, 2023, 10:36 p.m. UTC 
Spurred by Jivan's patch and a desire for cleaner testresults, I went 
ahead and make the stack_save_restore tests independent of the precise 
stack size by using a regexp.

Pushed to the trunk.

Jeff
commit e1f096a3cc96c71907cfbc7b8baf67a3d863cb6d
Author: Jeff Law <jlaw@ventanamicro.com>
Date:   Fri Aug 25 16:34:17 2023 -0600

    RISC-V: Make stack_save_restore tests more robust
    
    Spurred by Jivan's patch and a desire for cleaner testresults, I went ahead and
    make the stack_save_restore tests independent of the precise stack size by
    using a regexp.
    
    gcc/testsuite/
            * gcc.target/riscv/stack_save_restore_1.c: Robustify.
            * gcc.target/riscv/stack_save_restore_2.c: Robustify.

Comments

Patrick O'Neill Oct. 27, 2023, 5:34 p.m. UTC | #1 
On 8/25/23 15:36, Jeff Law wrote:
> Spurred by Jivan's patch and a desire for cleaner testresults, I went 
> ahead and make the stack_save_restore tests independent of the precise 
> stack size by using a regexp.
>
> Pushed to the trunk.
>
> Jeff

Hi Jeff, A recent change that I'm still bisecting [1] caused 
stack_save_restore_2.c to start failing.

Debug log:

Executing on host: /github/patrick-postcommit-runner-1/_work/gcc-postcommit-ci/gcc-postcommit-ci/riscv-gnu-toolchain/build/build-gcc-linux-stage2/gcc/xgcc -B/github/patrick-postcommit-runner-1/_work/gcc-postcommit-ci/gcc-postcommit-ci/riscv-gnu-toolchain/build/build-gcc-linux-stage2/gcc/  /github/patrick-postcommit-runner-1/_work/gcc-postcommit-ci/gcc-postcommit-ci/riscv-gnu-toolchain/gcc/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c  -march=rv32gcv -mabi=ilp32d -mcmodel=medlow   -fdiagnostics-plain-output    -O0  -march=rv32imafc -mabi=ilp32f -msave-restore -O2 -fno-schedule-insns -fno-schedule-insns2 -fno-unroll-loops -fno-peel-loops -fno-lto -S   -o stack_save_restore_2.s    (timeout = 600)
spawn -ignore SIGHUP /github/patrick-postcommit-runner-1/_work/gcc-postcommit-ci/gcc-postcommit-ci/riscv-gnu-toolchain/build/build-gcc-linux-stage2/gcc/xgcc -B/github/patrick-postcommit-runner-1/_work/gcc-postcommit-ci/gcc-postcommit-ci/riscv-gnu-toolchain/build/build-gcc-linux-stage2/gcc/ /github/patrick-postcommit-runner-1/_work/gcc-postcommit-ci/gcc-postcommit-ci/riscv-gnu-toolchain/gcc/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c -march=rv32gcv -mabi=ilp32d -mcmodel=medlow -fdiagnostics-plain-output -O0 -march=rv32imafc -mabi=ilp32f -msave-restore -O2 -fno-schedule-insns -fno-schedule-insns2 -fno-unroll-loops -fno-peel-loops -fno-lto -S -o stack_save_restore_2.s
PASS: gcc.target/riscv/stack_save_restore_2.c   -O0  (test for excess errors)
body: \tcall	t0,__riscv_save_(3|4)
\taddi	sp,sp,-[0-9]+
.*\tli	t0,-[0-9]+
\tadd	sp,sp,t0
.*\tli	t0,[0-9]+
\tadd	sp,sp,t0
.*\taddi	sp,sp,[0-9]+
\ttail	__riscv_restore_(3|4)

against: 	call	t0,__riscv_save_5
	addi	sp,sp,-2016
	fsw	fs0,2012(sp)
	fsw	fs1,2008(sp)
	fsw	fs2,2004(sp)
	fsw	fs3,2000(sp)
	fsw	fs4,1996(sp)
	li	t0,-12288
	add	sp,sp,t0
	call	getf
	fmv.s	fs1,fa0
	call	getf
	fmv.s	fs4,fa0
	call	getf
	fmv.s	fs3,fa0
	call	getf
	fmv.s	fs2,fa0
	li	s0,0
	fmv.s.x	fs0,zero
	lui	a5,%hi(.LC0)
	lw	s2,%lo(.LC0)(a5)
	lw	s3,%lo(.LC0+4)(a5)
	addi	s4,sp,1984
	li	s1,4096
	addi	s1,s1,-528
	call	my_getchar
	call	__floatsidf
	mv	a2,s2
	mv	a3,s3
	call	__muldf3
	call	__truncdfsf2
	slli	a5,s0,2
	add	a5,s4,a5
	fsw	fa0,-1984(a5)
	flw	fa5,-1984(a5)
	fadd.s	fs0,fs0,fa5
	addi	s0,s0,1
	bne	s0,s1,.L2
	fadd.s	fa5,fs1,fs0
	fadd.s	fa5,fa5,fs4
	fadd.s	fa5,fa5,fs3
	fadd.s	fa5,fa5,fs2
	fcvt.w.s a0,fa5,rtz
	li	t0,12288
	add	sp,sp,t0
	flw	fs0,2012(sp)
	flw	fs1,2008(sp)
	flw	fs2,2004(sp)
	flw	fs3,2000(sp)
	flw	fs4,1996(sp)
	addi	sp,sp,2016
	tail	__riscv_restore_5

FAIL: gcc.target/riscv/stack_save_restore_2.c   -O0   check-function-bodies bar

It looks like the issue is that your regex matches
__riscv_save_(3|4) where now gcc emits __riscv_restore_5.

Would it be OK to update the regex to also accept 5 (& are we going to 
bump into this again)?

diff --git a/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c 
b/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c
index 4c549cb11ae..bc95736cf8e 100644
--- a/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c
+++ b/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c
@@ -7,7 +7,7 @@ float getf();

  /*
  ** bar:
-**     call    t0,__riscv_save_(3|4)
+**     call    t0,__riscv_save_(3|4|5)
  **     addi    sp,sp,-[0-9]+
  **     ...
  **     li      t0,-[0-9]+
@@ -17,7 +17,7 @@ float getf();
  **     add     sp,sp,t0
  **     ...
  **     addi    sp,sp,[0-9]+
-**     tail    __riscv_restore_(3|4)
+**     tail    __riscv_restore_(3|4|5)
  */
  int bar()
  {

If we're going to run into this again, it might make sense to allow a 
wider range of numbers (up to 9):

diff --git a/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c 
b/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c
index 4c549cb11ae..1d5b950130e 100644
--- a/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c
+++ b/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c
@@ -7,7 +7,7 @@ float getf();

  /*
  ** bar:
-**     call    t0,__riscv_save_(3|4)
+**     call    t0,__riscv_save_([3-9])
  **     addi    sp,sp,-[0-9]+
  **     ...
  **     li      t0,-[0-9]+
@@ -17,7 +17,7 @@ float getf();
  **     add     sp,sp,t0
  **     ...
  **     addi    sp,sp,[0-9]+
-**     tail    __riscv_restore_(3|4)
+**     tail    __riscv_restore_([3-9])
  */
  int bar()
  {

Patrick

[1] The commit is in this range:
https://github.com/gcc-mirror/gcc/compare/a4ca8691333344cecc595d1af8b21e51f588e2f2...4d49685d671e4e604b2b873ada65aaac89348794
Jeff Law Oct. 27, 2023, 5:42 p.m. UTC | #2 
On 10/27/23 11:34, Patrick O'Neill wrote:
> 
> On 8/25/23 15:36, Jeff Law wrote:
>> Spurred by Jivan's patch and a desire for cleaner testresults, I went 
>> ahead and make the stack_save_restore tests independent of the precise 
>> stack size by using a regexp.
>>
>> Pushed to the trunk.
>>
>> Jeff
> 
> Hi Jeff, A recent change that I'm still bisecting [1] caused 
> stack_save_restore_2.c to start failing.
> 
> Debug log:
> 
> Executing on host: /github/patrick-postcommit-runner-1/_work/gcc-postcommit-ci/gcc-postcommit-ci/riscv-gnu-toolchain/build/build-gcc-linux-stage2/gcc/xgcc -B/github/patrick-postcommit-runner-1/_work/gcc-postcommit-ci/gcc-postcommit-ci/riscv-gnu-toolchain/build/build-gcc-linux-stage2/gcc/  /github/patrick-postcommit-runner-1/_work/gcc-postcommit-ci/gcc-postcommit-ci/riscv-gnu-toolchain/gcc/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c  -march=rv32gcv -mabi=ilp32d -mcmodel=medlow   -fdiagnostics-plain-output    -O0  -march=rv32imafc -mabi=ilp32f -msave-restore -O2 -fno-schedule-insns -fno-schedule-insns2 -fno-unroll-loops -fno-peel-loops -fno-lto -S   -o stack_save_restore_2.s    (timeout = 600)
> spawn -ignore SIGHUP /github/patrick-postcommit-runner-1/_work/gcc-postcommit-ci/gcc-postcommit-ci/riscv-gnu-toolchain/build/build-gcc-linux-stage2/gcc/xgcc -B/github/patrick-postcommit-runner-1/_work/gcc-postcommit-ci/gcc-postcommit-ci/riscv-gnu-toolchain/build/build-gcc-linux-stage2/gcc/ /github/patrick-postcommit-runner-1/_work/gcc-postcommit-ci/gcc-postcommit-ci/riscv-gnu-toolchain/gcc/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c -march=rv32gcv -mabi=ilp32d -mcmodel=medlow -fdiagnostics-plain-output -O0 -march=rv32imafc -mabi=ilp32f -msave-restore -O2 -fno-schedule-insns -fno-schedule-insns2 -fno-unroll-loops -fno-peel-loops -fno-lto -S -o stack_save_restore_2.s
> PASS: gcc.target/riscv/stack_save_restore_2.c   -O0  (test for excess errors)
> body: \tcall	t0,__riscv_save_(3|4)
> \taddi	sp,sp,-[0-9]+
> .*\tli	t0,-[0-9]+
> \tadd	sp,sp,t0
> .*\tli	t0,[0-9]+
> \tadd	sp,sp,t0
> .*\taddi	sp,sp,[0-9]+
> \ttail	__riscv_restore_(3|4)
> 
> against: 	call	t0,__riscv_save_5
> 	addi	sp,sp,-2016
> 	fsw	fs0,2012(sp)
> 	fsw	fs1,2008(sp)
> 	fsw	fs2,2004(sp)
> 	fsw	fs3,2000(sp)
> 	fsw	fs4,1996(sp)
> 	li	t0,-12288
> 	add	sp,sp,t0
> 	call	getf
> 	fmv.s	fs1,fa0
> 	call	getf
> 	fmv.s	fs4,fa0
> 	call	getf
> 	fmv.s	fs3,fa0
> 	call	getf
> 	fmv.s	fs2,fa0
> 	li	s0,0
> 	fmv.s.x	fs0,zero
> 	lui	a5,%hi(.LC0)
> 	lw	s2,%lo(.LC0)(a5)
> 	lw	s3,%lo(.LC0+4)(a5)
> 	addi	s4,sp,1984
> 	li	s1,4096
> 	addi	s1,s1,-528
> 	call	my_getchar
> 	call	__floatsidf
> 	mv	a2,s2
> 	mv	a3,s3
> 	call	__muldf3
> 	call	__truncdfsf2
> 	slli	a5,s0,2
> 	add	a5,s4,a5
> 	fsw	fa0,-1984(a5)
> 	flw	fa5,-1984(a5)
> 	fadd.s	fs0,fs0,fa5
> 	addi	s0,s0,1
> 	bne	s0,s1,.L2
> 	fadd.s	fa5,fs1,fs0
> 	fadd.s	fa5,fa5,fs4
> 	fadd.s	fa5,fa5,fs3
> 	fadd.s	fa5,fa5,fs2
> 	fcvt.w.s a0,fa5,rtz
> 	li	t0,12288
> 	add	sp,sp,t0
> 	flw	fs0,2012(sp)
> 	flw	fs1,2008(sp)
> 	flw	fs2,2004(sp)
> 	flw	fs3,2000(sp)
> 	flw	fs4,1996(sp)
> 	addi	sp,sp,2016
> 	tail	__riscv_restore_5
> 
> FAIL: gcc.target/riscv/stack_save_restore_2.c   -O0   check-function-bodies bar
> 
> It looks like the issue is that your regex matches
> __riscv_save_(3|4) where now gcc emits __riscv_restore_5.
> 
> Would it be OK to update the regex to also accept 5 (& are we going to 
> bump into this again)?
Thanks for looking at this -- my tester flagged them yesterday as well 
and I hadn't dug into them yet:

> Tests that now fail, but worked before (11 tests):
> 
> gcc.target/riscv/rv32i_zcmp.c   -Os   check-function-bodies test1
> gcc.target/riscv/rv32i_zcmp.c   -Os   check-function-bodies test2_step1_0_size
> gcc.target/riscv/rv32i_zcmp.c   -Os   check-function-bodies test3
> gcc.target/riscv/stack_save_restore_2.c   -O0   check-function-bodies bar
> gcc.target/riscv/stack_save_restore_2.c   -O1   check-function-bodies bar
> gcc.target/riscv/stack_save_restore_2.c   -O2   check-function-bodies bar
> gcc.target/riscv/stack_save_restore_2.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none   check-function-bodies bar
> gcc.target/riscv/stack_save_restore_2.c   -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects   check-function-bodies bar
> gcc.target/riscv/stack_save_restore_2.c   -O3 -fomit-frame-pointer -funroll-loops -fpeel-loops -ftracer -finline-functions   check-function-bodies bar
> gcc.target/riscv/stack_save_restore_2.c   -O3 -g   check-function-bodies bar
> gcc.target/riscv/stack_save_restore_2.c   -Os   check-function-bodies bar



Yes, I think accepting more cases here is quite reasonable.  In fact, 
you might just change it to look for __riscv_save_ without the numeric 
suffix.

jeff
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.target/riscv/stack_save_restore_1.c b/gcc/testsuite/gcc.target/riscv/stack_save_restore_1.c
index 255ce5f40c9..d8b0668a820 100644
--- a/gcc/testsuite/gcc.target/riscv/stack_save_restore_1.c
+++ b/gcc/testsuite/gcc.target/riscv/stack_save_restore_1.c
@@ -8,15 +8,15 @@  float getf();
 /*
 ** bar:
 **	call	t0,__riscv_save_(3|4)
-**	addi	sp,sp,-2032
+**	addi	sp,sp,-[0-9]+
 **	...
-**	li	t0,-12288
+**	li	t0,-[0-9]+
 **	add	sp,sp,t0
 **	...
-**	li	t0,12288
+**	li	t0,[0-9]+
 **	add	sp,sp,t0
 **	...
-**	addi	sp,sp,2032
+**	addi	sp,sp,[0-9]+
 **	tail	__riscv_restore_(3|4)
 */
 int bar()
diff --git a/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c b/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c
index 4ce5e0118a4..4c549cb11ae 100644
--- a/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c
+++ b/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c
@@ -8,15 +8,15 @@  float getf();
 /*
 ** bar:
 **	call	t0,__riscv_save_(3|4)
-**	addi	sp,sp,-2032
+**	addi	sp,sp,-[0-9]+
 **	...
-**	li	t0,-12288
+**	li	t0,-[0-9]+
 **	add	sp,sp,t0
 **	...
-**	li	t0,12288
+**	li	t0,[0-9]+
 **	add	sp,sp,t0
 **	...
-**	addi	sp,sp,2032
+**	addi	sp,sp,[0-9]+
 **	tail	__riscv_restore_(3|4)
 */
 int bar()