[V2] RISC-V: Ensure vector args and return use function stack to pass [PR110119]

Message ID 20230614115611.2227435-1-lehua.ding@rivai.ai
State Unresolved
Headers
Series [V2] RISC-V: Ensure vector args and return use function stack to pass [PR110119] |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Lehua Ding June 14, 2023, 11:56 a.m. UTC
  The V2 patch address comments from Juzhe, thanks.

Hi,
 
The reason for this bug is that in the case where the vector register is set
to a fixed length (with `--param=riscv-autovec-preference=fixed-vlmax` option),
TARGET_PASS_BY_REFERENCE thinks that variables of type vint32m1 can be passed
through two scalar registers, but when GCC calls FUNCTION_VALUE (call function
riscv_get_arg_info inside) it returns NULL_RTX. These two functions are not
unified. The current treatment is to pass all vector arguments and returns
through the function stack, and a new calling convention for vector registers
will be added in the future.
 
Best,
Lehua

        PR target/110119

gcc/ChangeLog:

        * config/riscv/riscv.cc (riscv_get_arg_info): Return NULL_RTX for vector mode
        (riscv_pass_by_reference): Return true for vector mode

gcc/testsuite/ChangeLog:

        * gcc.target/riscv/rvv/base/pr110119-1.c: New test.
        * gcc.target/riscv/rvv/base/pr110119-2.c: New test.

---
 gcc/config/riscv/riscv.cc                     | 17 ++++++++----
 .../gcc.target/riscv/rvv/base/pr110119-1.c    | 26 +++++++++++++++++++
 .../gcc.target/riscv/rvv/base/pr110119-2.c    | 26 +++++++++++++++++++
 3 files changed, 64 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c
 create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c
  

Comments

juzhe.zhong@rivai.ai June 14, 2023, 11:59 a.m. UTC | #1
LGTM now. Thanks for fixing it.

Good to see a Fix patch of the ICE before Vector ABI patch.
Let's wait for more comments.

Lehua Ding takes care of Vector ABI implementation and hopefully will send it soon.

Thanks.


juzhe.zhong@rivai.ai
 
From: Lehua Ding
Date: 2023-06-14 19:56
To: gcc-patches
CC: juzhe.zhong; rdapp.gcc; jeffreyalaw; palmer
Subject: [PATCH V2] RISC-V: Ensure vector args and return use function stack to pass [PR110119]
The V2 patch address comments from Juzhe, thanks.
 
Hi,
The reason for this bug is that in the case where the vector register is set
to a fixed length (with `--param=riscv-autovec-preference=fixed-vlmax` option),
TARGET_PASS_BY_REFERENCE thinks that variables of type vint32m1 can be passed
through two scalar registers, but when GCC calls FUNCTION_VALUE (call function
riscv_get_arg_info inside) it returns NULL_RTX. These two functions are not
unified. The current treatment is to pass all vector arguments and returns
through the function stack, and a new calling convention for vector registers
will be added in the future.
Best,
Lehua
 
        PR target/110119
 
gcc/ChangeLog:
 
        * config/riscv/riscv.cc (riscv_get_arg_info): Return NULL_RTX for vector mode
        (riscv_pass_by_reference): Return true for vector mode
 
gcc/testsuite/ChangeLog:
 
        * gcc.target/riscv/rvv/base/pr110119-1.c: New test.
        * gcc.target/riscv/rvv/base/pr110119-2.c: New test.
 
---
gcc/config/riscv/riscv.cc                     | 17 ++++++++----
.../gcc.target/riscv/rvv/base/pr110119-1.c    | 26 +++++++++++++++++++
.../gcc.target/riscv/rvv/base/pr110119-2.c    | 26 +++++++++++++++++++
3 files changed, 64 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c
 
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index dd5361c2bd2a..e5ae4e81b7a5 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -3915,13 +3915,13 @@ riscv_get_arg_info (struct riscv_arg_info *info, const CUMULATIVE_ARGS *cum,
       riscv_pass_in_vector_p (type);
     }
-  /* TODO: Currently, it will cause an ICE for --param
-     riscv-autovec-preference=fixed-vlmax. So, we just return NULL_RTX here
-     let GCC generate loads/stores. Ideally, we should either warn the user not
-     to use an RVV vector type as function argument or support the calling
-     convention directly.  */
+  /* All current vector arguments and return values are passed through the
+     function stack. Ideally, we should either warn the user not to use an RVV
+     vector type as function argument or support a calling convention
+     with better performance.  */
   if (riscv_v_ext_mode_p (mode))
     return NULL_RTX;
+
   if (named)
     {
       riscv_aggregate_field fields[2];
@@ -4106,6 +4106,13 @@ riscv_pass_by_reference (cumulative_args_t cum_v, const function_arg_info &arg)
return false;
     }
+  /* All current vector arguments and return values are passed through the
+     function stack. Ideally, we should either warn the user not to use an RVV
+     vector type as function argument or support a calling convention
+     with better performance.  */
+  if (riscv_v_ext_mode_p (arg.mode))
+    return true;
+
   /* Pass by reference if the data do not fit in two integer registers.  */
   return !IN_RANGE (size, 0, 2 * UNITS_PER_WORD);
}
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c
new file mode 100644
index 000000000000..f16502bcfeec
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gcv --param=riscv-autovec-preference=fixed-vlmax" } */
+
+#include "riscv_vector.h"
+
+typedef int8_t vnx2qi __attribute__ ((vector_size (2)));
+
+__attribute__ ((noipa)) vnx2qi
+f_vnx2qi (int8_t a, int8_t b, int8_t *out)
+{
+  vnx2qi v = {a, b};
+  return v;
+}
+
+__attribute__ ((noipa)) vnx2qi
+f_vnx2qi_2 (vnx2qi a, int8_t *out)
+{
+  return a;
+}
+
+__attribute__ ((noipa)) vint32m1_t
+f_vint32m1 (int8_t *a, int8_t *out)
+{
+  vint32m1_t v = *(vint32m1_t *) a;
+  return v;
+}
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c
new file mode 100644
index 000000000000..b233ff1e9040
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gczve32x --param=riscv-autovec-preference=fixed-vlmax" } */
+
+#include <stdint.h>
+#include "riscv_vector.h"
+
+__attribute__ ((noipa)) vint32m1x3_t
+foo1 (int32_t *in, int vl)
+{
+  vint32m1x3_t v = __riscv_vlseg3e32_v_i32m1x3 (in, vl);
+  return v;
+}
+
+__attribute__ ((noipa)) void
+foo2 (vint32m1x3_t a, int32_t *out, int vl)
+{
+  __riscv_vsseg3e32_v_i32m1x3 (out, a, vl);
+}
+
+__attribute__ ((noipa)) vint32m1x3_t
+foo3 (vint32m1x3_t a, int32_t *out, int32_t *in, int vl)
+{
+  __riscv_vsseg3e32_v_i32m1x3 (out, a, vl);
+  vint32m1x3_t v = __riscv_vlseg3e32_v_i32m1x3 (in, vl);
+  return v;
+}
-- 
2.36.3
  
juzhe.zhong@rivai.ai June 14, 2023, 12:01 p.m. UTC | #2
LGTM now. Thanks for fixing it.

Good to see a Fix patch of the ICE before Vector ABI patch.
Let's wait for more comments.

Lehua Ding takes care of Vector ABI implementation and hopefully will send it soon.

It seems the email of Jeff is wrong. CC Jeff .....for you.

Thanks.



juzhe.zhong@rivai.ai
 
From: Lehua Ding
Date: 2023-06-14 19:56
To: gcc-patches
CC: juzhe.zhong; rdapp.gcc; jeffreyalaw; palmer
Subject: [PATCH V2] RISC-V: Ensure vector args and return use function stack to pass [PR110119]
The V2 patch address comments from Juzhe, thanks.
 
Hi,
The reason for this bug is that in the case where the vector register is set
to a fixed length (with `--param=riscv-autovec-preference=fixed-vlmax` option),
TARGET_PASS_BY_REFERENCE thinks that variables of type vint32m1 can be passed
through two scalar registers, but when GCC calls FUNCTION_VALUE (call function
riscv_get_arg_info inside) it returns NULL_RTX. These two functions are not
unified. The current treatment is to pass all vector arguments and returns
through the function stack, and a new calling convention for vector registers
will be added in the future.
Best,
Lehua
 
        PR target/110119
 
gcc/ChangeLog:
 
        * config/riscv/riscv.cc (riscv_get_arg_info): Return NULL_RTX for vector mode
        (riscv_pass_by_reference): Return true for vector mode
 
gcc/testsuite/ChangeLog:
 
        * gcc.target/riscv/rvv/base/pr110119-1.c: New test.
        * gcc.target/riscv/rvv/base/pr110119-2.c: New test.
 
---
gcc/config/riscv/riscv.cc                     | 17 ++++++++----
.../gcc.target/riscv/rvv/base/pr110119-1.c    | 26 +++++++++++++++++++
.../gcc.target/riscv/rvv/base/pr110119-2.c    | 26 +++++++++++++++++++
3 files changed, 64 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c
 
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index dd5361c2bd2a..e5ae4e81b7a5 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -3915,13 +3915,13 @@ riscv_get_arg_info (struct riscv_arg_info *info, const CUMULATIVE_ARGS *cum,
       riscv_pass_in_vector_p (type);
     }
-  /* TODO: Currently, it will cause an ICE for --param
-     riscv-autovec-preference=fixed-vlmax. So, we just return NULL_RTX here
-     let GCC generate loads/stores. Ideally, we should either warn the user not
-     to use an RVV vector type as function argument or support the calling
-     convention directly.  */
+  /* All current vector arguments and return values are passed through the
+     function stack. Ideally, we should either warn the user not to use an RVV
+     vector type as function argument or support a calling convention
+     with better performance.  */
   if (riscv_v_ext_mode_p (mode))
     return NULL_RTX;
+
   if (named)
     {
       riscv_aggregate_field fields[2];
@@ -4106,6 +4106,13 @@ riscv_pass_by_reference (cumulative_args_t cum_v, const function_arg_info &arg)
return false;
     }
+  /* All current vector arguments and return values are passed through the
+     function stack. Ideally, we should either warn the user not to use an RVV
+     vector type as function argument or support a calling convention
+     with better performance.  */
+  if (riscv_v_ext_mode_p (arg.mode))
+    return true;
+
   /* Pass by reference if the data do not fit in two integer registers.  */
   return !IN_RANGE (size, 0, 2 * UNITS_PER_WORD);
}
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c
new file mode 100644
index 000000000000..f16502bcfeec
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gcv --param=riscv-autovec-preference=fixed-vlmax" } */
+
+#include "riscv_vector.h"
+
+typedef int8_t vnx2qi __attribute__ ((vector_size (2)));
+
+__attribute__ ((noipa)) vnx2qi
+f_vnx2qi (int8_t a, int8_t b, int8_t *out)
+{
+  vnx2qi v = {a, b};
+  return v;
+}
+
+__attribute__ ((noipa)) vnx2qi
+f_vnx2qi_2 (vnx2qi a, int8_t *out)
+{
+  return a;
+}
+
+__attribute__ ((noipa)) vint32m1_t
+f_vint32m1 (int8_t *a, int8_t *out)
+{
+  vint32m1_t v = *(vint32m1_t *) a;
+  return v;
+}
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c
new file mode 100644
index 000000000000..b233ff1e9040
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gczve32x --param=riscv-autovec-preference=fixed-vlmax" } */
+
+#include <stdint.h>
+#include "riscv_vector.h"
+
+__attribute__ ((noipa)) vint32m1x3_t
+foo1 (int32_t *in, int vl)
+{
+  vint32m1x3_t v = __riscv_vlseg3e32_v_i32m1x3 (in, vl);
+  return v;
+}
+
+__attribute__ ((noipa)) void
+foo2 (vint32m1x3_t a, int32_t *out, int vl)
+{
+  __riscv_vsseg3e32_v_i32m1x3 (out, a, vl);
+}
+
+__attribute__ ((noipa)) vint32m1x3_t
+foo3 (vint32m1x3_t a, int32_t *out, int32_t *in, int vl)
+{
+  __riscv_vsseg3e32_v_i32m1x3 (out, a, vl);
+  vint32m1x3_t v = __riscv_vlseg3e32_v_i32m1x3 (in, vl);
+  return v;
+}
-- 
2.36.3
  
juzhe.zhong@rivai.ai June 14, 2023, 12:03 p.m. UTC | #3
LGTM now. Thanks for fixing it.

Good to see a Fix patch of the ICE before Vector ABI patch.
Let's wait for more comments.

Lehua Ding takes care of Vector ABI implementation and hopefully will send it soon.

It seems the email of Jeff is wrong. CC Jeff .....for you.

Oh. I see Robin's email is also wrong. CC Robin too for you.... 

Thanks.



juzhe.zhong@rivai.ai
 
From: Lehua Ding
Date: 2023-06-14 19:56
To: gcc-patches
CC: juzhe.zhong; rdapp.gcc; jeffreyalaw; palmer
Subject: [PATCH V2] RISC-V: Ensure vector args and return use function stack to pass [PR110119]
The V2 patch address comments from Juzhe, thanks.
 
Hi,
The reason for this bug is that in the case where the vector register is set
to a fixed length (with `--param=riscv-autovec-preference=fixed-vlmax` option),
TARGET_PASS_BY_REFERENCE thinks that variables of type vint32m1 can be passed
through two scalar registers, but when GCC calls FUNCTION_VALUE (call function
riscv_get_arg_info inside) it returns NULL_RTX. These two functions are not
unified. The current treatment is to pass all vector arguments and returns
through the function stack, and a new calling convention for vector registers
will be added in the future.
Best,
Lehua
 
        PR target/110119
 
gcc/ChangeLog:
 
        * config/riscv/riscv.cc (riscv_get_arg_info): Return NULL_RTX for vector mode
        (riscv_pass_by_reference): Return true for vector mode
 
gcc/testsuite/ChangeLog:
 
        * gcc.target/riscv/rvv/base/pr110119-1.c: New test.
        * gcc.target/riscv/rvv/base/pr110119-2.c: New test.
 
---
gcc/config/riscv/riscv.cc                     | 17 ++++++++----
.../gcc.target/riscv/rvv/base/pr110119-1.c    | 26 +++++++++++++++++++
.../gcc.target/riscv/rvv/base/pr110119-2.c    | 26 +++++++++++++++++++
3 files changed, 64 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c
 
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index dd5361c2bd2a..e5ae4e81b7a5 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -3915,13 +3915,13 @@ riscv_get_arg_info (struct riscv_arg_info *info, const CUMULATIVE_ARGS *cum,
       riscv_pass_in_vector_p (type);
     }
-  /* TODO: Currently, it will cause an ICE for --param
-     riscv-autovec-preference=fixed-vlmax. So, we just return NULL_RTX here
-     let GCC generate loads/stores. Ideally, we should either warn the user not
-     to use an RVV vector type as function argument or support the calling
-     convention directly.  */
+  /* All current vector arguments and return values are passed through the
+     function stack. Ideally, we should either warn the user not to use an RVV
+     vector type as function argument or support a calling convention
+     with better performance.  */
   if (riscv_v_ext_mode_p (mode))
     return NULL_RTX;
+
   if (named)
     {
       riscv_aggregate_field fields[2];
@@ -4106,6 +4106,13 @@ riscv_pass_by_reference (cumulative_args_t cum_v, const function_arg_info &arg)
return false;
     }
+  /* All current vector arguments and return values are passed through the
+     function stack. Ideally, we should either warn the user not to use an RVV
+     vector type as function argument or support a calling convention
+     with better performance.  */
+  if (riscv_v_ext_mode_p (arg.mode))
+    return true;
+
   /* Pass by reference if the data do not fit in two integer registers.  */
   return !IN_RANGE (size, 0, 2 * UNITS_PER_WORD);
}
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c
new file mode 100644
index 000000000000..f16502bcfeec
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gcv --param=riscv-autovec-preference=fixed-vlmax" } */
+
+#include "riscv_vector.h"
+
+typedef int8_t vnx2qi __attribute__ ((vector_size (2)));
+
+__attribute__ ((noipa)) vnx2qi
+f_vnx2qi (int8_t a, int8_t b, int8_t *out)
+{
+  vnx2qi v = {a, b};
+  return v;
+}
+
+__attribute__ ((noipa)) vnx2qi
+f_vnx2qi_2 (vnx2qi a, int8_t *out)
+{
+  return a;
+}
+
+__attribute__ ((noipa)) vint32m1_t
+f_vint32m1 (int8_t *a, int8_t *out)
+{
+  vint32m1_t v = *(vint32m1_t *) a;
+  return v;
+}
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c
new file mode 100644
index 000000000000..b233ff1e9040
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gczve32x --param=riscv-autovec-preference=fixed-vlmax" } */
+
+#include <stdint.h>
+#include "riscv_vector.h"
+
+__attribute__ ((noipa)) vint32m1x3_t
+foo1 (int32_t *in, int vl)
+{
+  vint32m1x3_t v = __riscv_vlseg3e32_v_i32m1x3 (in, vl);
+  return v;
+}
+
+__attribute__ ((noipa)) void
+foo2 (vint32m1x3_t a, int32_t *out, int vl)
+{
+  __riscv_vsseg3e32_v_i32m1x3 (out, a, vl);
+}
+
+__attribute__ ((noipa)) vint32m1x3_t
+foo3 (vint32m1x3_t a, int32_t *out, int32_t *in, int vl)
+{
+  __riscv_vsseg3e32_v_i32m1x3 (out, a, vl);
+  vint32m1x3_t v = __riscv_vlseg3e32_v_i32m1x3 (in, vl);
+  return v;
+}
-- 
2.36.3
  
Robin Dapp June 14, 2023, 12:05 p.m. UTC | #4
> Oh. I see Robin's email is also wrong. CC Robin too for you.... 

It still arrived via the mailing list ;)

> Good to see a Fix patch of the ICE before Vector ABI patch.
> Let's wait for more comments.

LGTM, this way I don't even need to rewrite my tests.

Regards
 Robin
  
Jeff Law June 14, 2023, 6:59 p.m. UTC | #5
On 6/14/23 06:01, juzhe.zhong@rivai.ai wrote:
> LGTM now. Thanks for fixing it.
> 
> Good to see a Fix patch of the ICE before Vector ABI patch.
> Let's wait for more comments.
> 
> Lehua Ding takes care of Vector ABI implementation and hopefully will 
> send it soon.
> 
> It seems the email of Jeff is wrong. CC Jeff .....for you.
The gmail address is fine.  I tend to use that for most of my upstream 
email interactions so that my work inbox is marginally decluttered.  And 
I'm also on gcc-patches, so I would have received it through that route 
as well.

jeff
  
Jeff Law June 14, 2023, 7:05 p.m. UTC | #6
On 6/14/23 06:05, Robin Dapp via Gcc-patches wrote:
>> Oh. I see Robin's email is also wrong. CC Robin too for you....
> 
> It still arrived via the mailing list ;)
> 
>> Good to see a Fix patch of the ICE before Vector ABI patch.
>> Let's wait for more comments.
> 
> LGTM, this way I don't even need to rewrite my tests.
I think Palmer wanted to include a pointer to the psabi MR, so we should 
probably include that in a comment.  So OK with that in a comment.

I think there was talk of having this all be hidden behind a flag, but 
given it's an ICE on vector types, I don't mind just defining something 
for now to fix the ICE and give psabi time to finalize that spec.

This was also a good reminder that the vector work can't really be 
complete until we have the psabi updates in place and implemented.   The 
efforts can obviously continue in parallel, but it's a dependency worth 
noting in the RISE context.

Jeff
  
Jeff Law June 14, 2023, 7:07 p.m. UTC | #7
On 6/14/23 05:56, Lehua Ding wrote:
> The V2 patch address comments from Juzhe, thanks.
> 
> Hi,
>   
> The reason for this bug is that in the case where the vector register is set
> to a fixed length (with `--param=riscv-autovec-preference=fixed-vlmax` option),
> TARGET_PASS_BY_REFERENCE thinks that variables of type vint32m1 can be passed
> through two scalar registers, but when GCC calls FUNCTION_VALUE (call function
> riscv_get_arg_info inside) it returns NULL_RTX. These two functions are not
> unified. The current treatment is to pass all vector arguments and returns
> through the function stack, and a new calling convention for vector registers
> will be added in the future.
>   
> Best,
> Lehua
> 
>          PR target/110119
> 
> gcc/ChangeLog:
> 
>          * config/riscv/riscv.cc (riscv_get_arg_info): Return NULL_RTX for vector mode
>          (riscv_pass_by_reference): Return true for vector mode
> 
> gcc/testsuite/ChangeLog:
> 
>          * gcc.target/riscv/rvv/base/pr110119-1.c: New test.
>          * gcc.target/riscv/rvv/base/pr110119-2.c: New test.
And just to be clear, I've asked for a minor comment update.  The usual 
procedure is to go ahead and post a V3.  In this case I'll also give 
that V3 pre-approval.  So no need to wait for additional acks.  Post it 
and it can be committed immediately.

jeff
  
Li, Pan2 via Gcc-patches June 15, 2023, 1:34 a.m. UTC | #8
Committed with the comment update,, thanks Jeff and Juzhe.

Pan

-----Original Message-----
From: Gcc-patches <gcc-patches-bounces+pan2.li=intel.com@gcc.gnu.org> On Behalf Of Jeff Law via Gcc-patches
Sent: Thursday, June 15, 2023 3:08 AM
To: Lehua Ding <lehua.ding@rivai.ai>; gcc-patches@gcc.gnu.org
Cc: juzhe.zhong@rivai.ai; rdapp.gcc@gamil.com; jeffreyalaw@gamil.com; palmer@rivosinc.com
Subject: Re: [PATCH V2] RISC-V: Ensure vector args and return use function stack to pass [PR110119]



On 6/14/23 05:56, Lehua Ding wrote:
> The V2 patch address comments from Juzhe, thanks.
> 
> Hi,
>   
> The reason for this bug is that in the case where the vector register 
> is set to a fixed length (with 
> `--param=riscv-autovec-preference=fixed-vlmax` option), 
> TARGET_PASS_BY_REFERENCE thinks that variables of type vint32m1 can be 
> passed through two scalar registers, but when GCC calls FUNCTION_VALUE 
> (call function riscv_get_arg_info inside) it returns NULL_RTX. These 
> two functions are not unified. The current treatment is to pass all 
> vector arguments and returns through the function stack, and a new calling convention for vector registers will be added in the future.
>   
> Best,
> Lehua
> 
>          PR target/110119
> 
> gcc/ChangeLog:
> 
>          * config/riscv/riscv.cc (riscv_get_arg_info): Return NULL_RTX for vector mode
>          (riscv_pass_by_reference): Return true for vector mode
> 
> gcc/testsuite/ChangeLog:
> 
>          * gcc.target/riscv/rvv/base/pr110119-1.c: New test.
>          * gcc.target/riscv/rvv/base/pr110119-2.c: New test.
And just to be clear, I've asked for a minor comment update.  The usual procedure is to go ahead and post a V3.  In this case I'll also give that V3 pre-approval.  So no need to wait for additional acks.  Post it and it can be committed immediately.

jeff
  

Patch

diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index dd5361c2bd2a..e5ae4e81b7a5 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -3915,13 +3915,13 @@  riscv_get_arg_info (struct riscv_arg_info *info, const CUMULATIVE_ARGS *cum,
       riscv_pass_in_vector_p (type);
     }
 
-  /* TODO: Currently, it will cause an ICE for --param
-     riscv-autovec-preference=fixed-vlmax. So, we just return NULL_RTX here
-     let GCC generate loads/stores. Ideally, we should either warn the user not
-     to use an RVV vector type as function argument or support the calling
-     convention directly.  */
+  /* All current vector arguments and return values are passed through the
+     function stack. Ideally, we should either warn the user not to use an RVV
+     vector type as function argument or support a calling convention
+     with better performance.  */
   if (riscv_v_ext_mode_p (mode))
     return NULL_RTX;
+
   if (named)
     {
       riscv_aggregate_field fields[2];
@@ -4106,6 +4106,13 @@  riscv_pass_by_reference (cumulative_args_t cum_v, const function_arg_info &arg)
 	return false;
     }
 
+  /* All current vector arguments and return values are passed through the
+     function stack. Ideally, we should either warn the user not to use an RVV
+     vector type as function argument or support a calling convention
+     with better performance.  */
+  if (riscv_v_ext_mode_p (arg.mode))
+    return true;
+
   /* Pass by reference if the data do not fit in two integer registers.  */
   return !IN_RANGE (size, 0, 2 * UNITS_PER_WORD);
 }
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c
new file mode 100644
index 000000000000..f16502bcfeec
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-1.c
@@ -0,0 +1,26 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gcv --param=riscv-autovec-preference=fixed-vlmax" } */
+
+#include "riscv_vector.h"
+
+typedef int8_t vnx2qi __attribute__ ((vector_size (2)));
+
+__attribute__ ((noipa)) vnx2qi
+f_vnx2qi (int8_t a, int8_t b, int8_t *out)
+{
+  vnx2qi v = {a, b};
+  return v;
+}
+
+__attribute__ ((noipa)) vnx2qi
+f_vnx2qi_2 (vnx2qi a, int8_t *out)
+{
+  return a;
+}
+
+__attribute__ ((noipa)) vint32m1_t
+f_vint32m1 (int8_t *a, int8_t *out)
+{
+  vint32m1_t v = *(vint32m1_t *) a;
+  return v;
+}
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c
new file mode 100644
index 000000000000..b233ff1e9040
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/rvv/base/pr110119-2.c
@@ -0,0 +1,26 @@ 
+/* { dg-do compile } */
+/* { dg-options "-march=rv64gczve32x --param=riscv-autovec-preference=fixed-vlmax" } */
+
+#include <stdint.h>
+#include "riscv_vector.h"
+
+__attribute__ ((noipa)) vint32m1x3_t
+foo1 (int32_t *in, int vl)
+{
+  vint32m1x3_t v = __riscv_vlseg3e32_v_i32m1x3 (in, vl);
+  return v;
+}
+
+__attribute__ ((noipa)) void
+foo2 (vint32m1x3_t a, int32_t *out, int vl)
+{
+  __riscv_vsseg3e32_v_i32m1x3 (out, a, vl);
+}
+
+__attribute__ ((noipa)) vint32m1x3_t
+foo3 (vint32m1x3_t a, int32_t *out, int32_t *in, int vl)
+{
+  __riscv_vsseg3e32_v_i32m1x3 (out, a, vl);
+  vint32m1x3_t v = __riscv_vlseg3e32_v_i32m1x3 (in, vl);
+  return v;
+}