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

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

[Lehua Ding]

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

Re: [PATCH] RISC-V: Fix testcase failed when default -mcmodel=medany

[juzhe.zhong]

[-- Attachment #1: Type: text/plain, Size: 1607 bytes --]

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
 

Re: [PATCH] RISC-V: Fix testcase failed when default -mcmodel=medany

[Robin Dapp]

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

Re: [PATCH] RISC-V: Fix testcase failed when default -mcmodel=medany

[Lehua Ding]

[-- Attachment #1: Type: text/plain, Size: 711 bytes --]

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

Re: [PATCH] RISC-V: Fix testcase failed when default -mcmodel=medany

[Robin Dapp]

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

Re: [PATCH] RISC-V: Fix testcase failed when default -mcmodel=medany

[Lehua Ding]

Hi Robin,

> In general I'm fine with this small change of course, I just wonder if
> the testcase 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?

I think you are rigth, I would like to remove the `-mcmodel=medany` option and
relax assert from `__riscv_save/restore_4` to `__riscv_save/restore_(3|4)` to let
this testcase not brittle on any -mcmodel.  Then I'm also going to add another
testcase (I dont known how to run -march=rv32imafc and -march=rv64imafc on
the same testcase) that uses -march=rv64imafc.

Removing scheduling option will result in a change in the order of the assert
assembly, and I don't feel like removing it because the order may be different for
different microarchitectures.

Best,
Lehua

V2 patch:

gcc/testsuite/ChangeLog:

	* gcc.target/riscv/stack_save_restore.c: Moved to...
	* gcc.target/riscv/stack_save_restore_2.c: ...here.
	* gcc.target/riscv/stack_save_restore_1.c: New test.

---
 .../gcc.target/riscv/stack_save_restore_1.c   | 40 +++++++++++++++++++
 ..._save_restore.c => stack_save_restore_2.c} |  6 +--
 2 files changed, 43 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/stack_save_restore_1.c
 rename gcc/testsuite/gcc.target/riscv/{stack_save_restore.c => stack_save_restore_2.c} (90%)

diff --git a/gcc/testsuite/gcc.target/riscv/stack_save_restore_1.c b/gcc/testsuite/gcc.target/riscv/stack_save_restore_1.c
new file mode 100644
index 00000000000..255ce5f40c9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/stack_save_restore_1.c
@@ -0,0 +1,40 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64imafc -mabi=lp64f -msave-restore -O2 -fno-schedule-insns -fno-schedule-insns2 -fno-unroll-loops -fno-peel-loops -fno-lto" } */
+/* { dg-final { check-function-bodies "**" "" } } */
+
+char my_getchar();
+float getf();
+
+/*
+** bar:
+**	call	t0,__riscv_save_(3|4)
+**	addi	sp,sp,-2032
+**	...
+**	li	t0,-12288
+**	add	sp,sp,t0
+**	...
+**	li	t0,12288
+**	add	sp,sp,t0
+**	...
+**	addi	sp,sp,2032
+**	tail	__riscv_restore_(3|4)
+*/
+int bar()
+{
+  float volatile farray[3568];
+
+  float sum = 0;
+  float f1 = getf();
+  float f2 = getf();
+  float f3 = getf();
+  float f4 = getf();
+
+  for (int i = 0; i < 3568; i++)
+  {
+    farray[i] = my_getchar() * 1.2;
+    sum += farray[i];
+  }
+
+  return sum + f1 + f2 + f3 + f4;
+}
+
diff --git a/gcc/testsuite/gcc.target/riscv/stack_save_restore.c b/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c
similarity index 90%
rename from gcc/testsuite/gcc.target/riscv/stack_save_restore.c
rename to gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c
index 522e706cfbf..4ce5e0118a4 100644
--- a/gcc/testsuite/gcc.target/riscv/stack_save_restore.c
+++ b/gcc/testsuite/gcc.target/riscv/stack_save_restore_2.c
@@ -6,8 +6,8 @@ char my_getchar();
 float getf();
 
 /*
-**bar:
-**	call	t0,__riscv_save_4
+** bar:
+**	call	t0,__riscv_save_(3|4)
 **	addi	sp,sp,-2032
 **	...
 **	li	t0,-12288
@@ -17,7 +17,7 @@ float getf();
 **	add	sp,sp,t0
 **	...
 **	addi	sp,sp,2032
-**	tail	__riscv_restore_4
+**	tail	__riscv_restore_(3|4)
 */
 int bar()
 {
-- 
2.36.3

Re: [PATCH] RISC-V: Fix testcase failed when default -mcmodel=medany

[Robin Dapp]

Hi Lehua,

> I think you are rigth, I would like to remove the `-mcmodel=medany` option and
> relax assert from `__riscv_save/restore_4` to `__riscv_save/restore_(3|4)` to let
> this testcase not brittle on any -mcmodel.  Then I'm also going to add another
> testcase (I dont known how to run -march=rv32imafc and -march=rv64imafc on
> the same testcase) that uses -march=rv64imafc.

Thanks, ideally we'd have a common include that contains the code
but I don't think we need to stress on that now.  At a later point,
we'd ideally have no rv32/rv64 ever in the options (apart from
specific bugs) and just run the test suite for both so only one file
would be needed.  But we don't have that yet, so LGTM.

Regards
 Robin

Re: [PATCH] RISC-V: Fix testcase failed when default -mcmodel=medany

[=?gb18030?B?TGVodWEgRGluZw==?=]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb18030", Size: 1544 bytes --]

Committed V2 patch, thank you so much.




------------------ Original ------------------
From:                                                                                                                        "Robin Dapp"                                                                                    <gcc-patches@gcc.gnu.org&gt;;
Date:&nbsp;Wed, Jul 19, 2023 02:03 PM
To:&nbsp;"Lehua Ding"<lehua.ding@rivai.ai&gt;;"gcc-patches"<gcc-patches@gcc.gnu.org&gt;;
Cc:&nbsp;"rdapp.gcc"<rdapp.gcc@gmail.com&gt;;"juzhe.zhong"<juzhe.zhong@rivai.ai&gt;;"kito.cheng"<kito.cheng@gmail.com&gt;;"palmer"<palmer@rivosinc.com&gt;;"jeffreyalaw"<jeffreyalaw@gmail.com&gt;;
Subject:&nbsp;Re: [PATCH] RISC-V: Fix testcase failed when default -mcmodel=medany



Hi Lehua,

&gt; I think you are rigth, I would like to remove the `-mcmodel=medany` option and
&gt; relax assert from `__riscv_save/restore_4` to `__riscv_save/restore_(3|4)` to let
&gt; this testcase not brittle on any -mcmodel.&nbsp; Then I'm also going to add another
&gt; testcase (I dont known how to run -march=rv32imafc and -march=rv64imafc on
&gt; the same testcase) that uses -march=rv64imafc.

Thanks, ideally we'd have a common include that contains the code
but I don't think we need to stress on that now.&nbsp; At a later point,
we'd ideally have no rv32/rv64 ever in the options (apart from
specific bugs) and just run the test suite for both so only one file
would be needed.&nbsp; But we don't have that yet, so LGTM.

Regards
&nbsp;Robin