diff mbox series

RISC-V: Fix testcase failed when default -mcmodel=medany

Message ID 20230718074249.236825-1-lehua.ding@rivai.ai
State Accepted
Headers
Series RISC-V: Fix testcase failed when default -mcmodel=medany |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Lehua Ding July 18, 2023, 7:42 a.m. UTC 
  Hi,

This patch fix testcase failed when I build RISC-V GCC with -mcmodel=medany
as default. If set to medany, stack_save_restore.c testcase will fail because of
the reduced use of s3 registers in assembly (thus calling __riscv_save/store_3
instead of __riscv_save/store_4). Explicitly add -mcmodel=medlow to solve this
problem.

Best,
Lehua

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/stack_save_restore.c: Add -mcmodel=medlow

---
 gcc/testsuite/gcc.target/riscv/stack_save_restore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

juzhe.zhong@rivai.ai July 18, 2023, 7:51 a.m. UTC | #1 
Not familiar with this stuff. 
I leave it other RISC-V folks to review.



juzhe.zhong@rivai.ai
 
From: Lehua Ding
Date: 2023-07-18 15:42
To: gcc-patches
CC: juzhe.zhong; rdapp.gcc; kito.cheng; palmer; jeffreyalaw
Subject: [PATCH] RISC-V: Fix testcase failed when default -mcmodel=medany
Hi,
 
This patch fix testcase failed when I build RISC-V GCC with -mcmodel=medany
as default. If set to medany, stack_save_restore.c testcase will fail because of
the reduced use of s3 registers in assembly (thus calling __riscv_save/store_3
instead of __riscv_save/store_4). Explicitly add -mcmodel=medlow to solve this
problem.
 
Best,
Lehua
 
gcc/testsuite/ChangeLog:
 
* gcc.target/riscv/stack_save_restore.c: Add -mcmodel=medlow
 
---
gcc/testsuite/gcc.target/riscv/stack_save_restore.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
 
diff --git a/gcc/testsuite/gcc.target/riscv/stack_save_restore.c b/gcc/testsuite/gcc.target/riscv/stack_save_restore.c
index 522e706cfbf..a2430783474 100644
--- a/gcc/testsuite/gcc.target/riscv/stack_save_restore.c
+++ b/gcc/testsuite/gcc.target/riscv/stack_save_restore.c
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-options "-march=rv32imafc -mabi=ilp32f -msave-restore -O2 -fno-schedule-insns -fno-schedule-insns2 -fno-unroll-loops -fno-peel-loops -fno-lto" } */
+/* { dg-options "-march=rv32imafc -mabi=ilp32f -msave-restore -O2 -fno-schedule-insns -fno-schedule-insns2 -fno-unroll-loops -fno-peel-loops -fno-lto -mcmodel=medlow" } */
/* { dg-final { check-function-bodies "**" "" } } */
char my_getchar();
-- 
2.36.1
Robin Dapp July 18, 2023, 8:18 a.m. UTC | #2 
Hi Lehua,

> This patch fix testcase failed when I build RISC-V GCC with -mcmodel=medany
> as default. If set to medany, stack_save_restore.c testcase will fail because of
> the reduced use of s3 registers in assembly (thus calling __riscv_save/store_3
> instead of __riscv_save/store_4). Explicitly add -mcmodel=medlow to solve this
> problem.

Wouldn't you rather want to adjust the test to not check for one register
number but 3 or 4 instead?  There might be future changes in default behavior
that would invalidate the test as well.

Regards
 Robin
Lehua Ding July 18, 2023, 8:43 a.m. UTC | #3 
Hi Robin,


> Wouldn't you rather want to adjust the test to not check for one register
> number but 3 or 4 instead?

I think the purpose of this testcase is to check whether the modifications to
the stack frame are as expected, so it is necessary to specify exactly whether
three or four registers are saved. But I think its need to add another testcase
which use another option -mcmodel=medany for coverage.


> There might be future changes in default behavior
> that would invalidate the test as well.

Because -mcmodel is explicitly specified in the testcase, future changes
to the default value of -mcmodel will not cause the test case to fail.


Best,
Lehua
Robin Dapp July 18, 2023, 10:03 a.m. UTC | #4 
Hi Lehua,

> I think the purpose of this testcase is to check whether the modifications to
> the stack frame are as expected, so it is necessary to specify exactly whether
> three or four registers are saved. But I think its need to add another testcase
> which use another option -mcmodel=medany for coverage.

In general I'm fine with this small change of course, I just wonder if
the test case is not brittle anyway.  From what I can tell the respective
change is independent of the actual number of registers so maybe it's enough to
not compare the fully body but just make sure the addis are not present?
That way, the test could also work for -march=rv64 (which saves one
register less anyway regardless of mcmodel - but the change still helps)
or maybe even with instruction scheduling.  Would you mind checking this still?
Thanks.

If it turns out not to be possible, let's stick with the medany fix for now
and add a TODO.

Regards
 Robin
diff mbox series

Patch

diff --git a/gcc/testsuite/gcc.target/riscv/stack_save_restore.c b/gcc/testsuite/gcc.target/riscv/stack_save_restore.c
index 522e706cfbf..a2430783474 100644
--- a/gcc/testsuite/gcc.target/riscv/stack_save_restore.c
+++ b/gcc/testsuite/gcc.target/riscv/stack_save_restore.c
@@ -1,5 +1,5 @@ 
 /* { dg-do compile } */
-/* { dg-options "-march=rv32imafc -mabi=ilp32f -msave-restore -O2 -fno-schedule-insns -fno-schedule-insns2 -fno-unroll-loops -fno-peel-loops -fno-lto" } */
+/* { dg-options "-march=rv32imafc -mabi=ilp32f -msave-restore -O2 -fno-schedule-insns -fno-schedule-insns2 -fno-unroll-loops -fno-peel-loops -fno-lto -mcmodel=medlow" } */
 /* { dg-final { check-function-bodies "**" "" } } */
 
 char my_getchar();