* [PATCH v2] RISC-V: Enable Hoist to GCSE simple constants
@ 2023-08-25 5:16 Vineet Gupta
2023-08-25 18:50 ` Jeff Law
0 siblings, 1 reply; 3+ messages in thread
From: Vineet Gupta @ 2023-08-25 5:16 UTC (permalink / raw)
To: gcc-patches
Cc: kito.cheng, Jeff Law, Palmer Dabbelt, gnu-toolchain, Vineet Gupta
Hoist want_to_gcse_p () calls rtx_cost () to compute max distance for
hoist candidates. For a simple const (say 6 which needs seperate insn "LI 6")
backend currently returns 0, causing Hoist to bail and elide GCSE.
Note that constants requiring more than 1 insns to setup were working
fine since riscv_rtx_costs () was returning non-zero (although that
itself might need refining: see bugzilla 111139).
To keep testsuite parity, some V tests need updating which started failing
in the new costing regime.
gcc/ChangeLog:
* gcc/config/riscv.cc (riscv_rtx_costs): Adjust const_int cost.
Add some comments about different constants handling.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/gcse-const.c: New Test
* gcc.target/riscv/rvv/vsetvl/vlmax_conflict-7.c: Remove test
for Jump.
* gcc.target/riscv/rvv/vsetvl/vlmax_conflict-8.c: Ditto.
Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
Changes since v1
- Simplified code under case CONST.
- Added some comments for handling of CONST_INT in 2 places.
---
gcc/config/riscv/riscv.cc | 18 +++++++++---------
gcc/testsuite/gcc.target/riscv/gcse-const.c | 13 +++++++++++++
.../riscv/rvv/vsetvl/vlmax_conflict-7.c | 1 -
.../riscv/rvv/vsetvl/vlmax_conflict-8.c | 1 -
4 files changed, 22 insertions(+), 11 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/gcse-const.c
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 13166d19619c..98a46b00ceb5 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -2490,6 +2490,8 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN
switch (GET_CODE (x))
{
case CONST_INT:
+ /* trivial constants checked using OUTER_CODE in case they are
+ encodable in insn itself w/o need for additional insn(s). */
if (riscv_immediate_operand_p (outer_code, INTVAL (x)))
{
*total = 0;
@@ -2507,17 +2509,15 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN
/* Fall through. */
case CONST:
+ /* Non trivial CONST_INT Fall through: check if need multiple insns. */
if ((cost = riscv_const_insns (x)) > 0)
{
- /* If the constant is likely to be stored in a GPR, SETs of
- single-insn constants are as cheap as register sets; we
- never want to CSE them. */
- if (cost == 1 && outer_code == SET)
- *total = 0;
- /* When we load a constant more than once, it usually is better
- to duplicate the last operation in the sequence than to CSE
- the constant itself. */
- else if (outer_code == SET || GET_MODE (x) == VOIDmode)
+ /* 1. Hoist will GCSE constants only if TOTAL returned is non-zero.
+ 2. For constants loaded more than once, the approach so far has
+ been to duplicate the operation than to CSE the constant.
+ 3. TODO: make cost more accurate specially if riscv_const_insns
+ returns > 1. */
+ if (outer_code == SET || GET_MODE (x) == VOIDmode)
*total = COSTS_N_INSNS (1);
}
else /* The instruction will be fetched from the constant pool. */
diff --git a/gcc/testsuite/gcc.target/riscv/gcse-const.c b/gcc/testsuite/gcc.target/riscv/gcse-const.c
new file mode 100644
index 000000000000..b04707ce9745
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/gcse-const.c
@@ -0,0 +1,13 @@
+/* Slightly modified copy of gcc.target/arm/pr40956.c. */
+/* { dg-options "-Os" } */
+/* Make sure the constant "6" is loaded into register only once. */
+/* { dg-final { scan-assembler-times "\tli.*6" 1 } } */
+
+int foo(int p, int* q)
+{
+ if (p!=9)
+ *q = 6;
+ else
+ *(q+1) = 6;
+ return 3;
+}
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-7.c b/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-7.c
index 60ad108666f8..085ca9db8542 100644
--- a/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-7.c
+++ b/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-7.c
@@ -21,5 +21,4 @@ void f (int32_t * restrict in, int32_t * restrict out, size_t n, size_t cond, si
}
/* { dg-final { scan-assembler-times {vsetvli} 4 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
-/* { dg-final { scan-assembler-times {j\s+\.L[0-9]+\s+\.L[0-9]+:\s+vlm\.v} 1 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
/* { dg-final { scan-assembler-times {vsetvli\s+[a-x0-9]+,\s*zero,\s*e8,\s*m8,\s*t[au],\s*m[au]} 3 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-8.c b/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-8.c
index 7b9574cc332d..fbca9b00e54e 100644
--- a/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-8.c
+++ b/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-8.c
@@ -21,7 +21,6 @@ void f (int32_t * restrict in, int32_t * restrict out, size_t n, size_t cond, si
}
/* { dg-final { scan-assembler-times {vsetvli} 5 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
-/* { dg-final { scan-assembler-times {j\s+\.L[0-9]+\s+\.L[0-9]+:\s+vlm\.v} 1 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
/* { dg-final { scan-assembler-times {vsetvli\s+[a-x0-9]+,\s*zero,\s*e8,\s*m8,\s*t[au],\s*m[au]} 3 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
/* { dg-final { scan-assembler-times {vsetvli\s+[a-x0-9]+,\s*zero,\s*e8,\s*mf8,\s*t[au],\s*m[au]} 1 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
/* { dg-final { scan-assembler-times {vsetvli\s+[a-x0-9]+,\s*zero,\s*e16,\s*mf2,\s*t[au],\s*m[au]} 1 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
--
2.34.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] RISC-V: Enable Hoist to GCSE simple constants
2023-08-25 5:16 [PATCH v2] RISC-V: Enable Hoist to GCSE simple constants Vineet Gupta
@ 2023-08-25 18:50 ` Jeff Law
2023-08-25 19:24 ` [Committed] " Vineet Gupta
0 siblings, 1 reply; 3+ messages in thread
From: Jeff Law @ 2023-08-25 18:50 UTC (permalink / raw)
To: Vineet Gupta, gcc-patches; +Cc: kito.cheng, Palmer Dabbelt, gnu-toolchain
On 8/24/23 23:16, Vineet Gupta wrote:
> Hoist want_to_gcse_p () calls rtx_cost () to compute max distance for
> hoist candidates. For a simple const (say 6 which needs seperate insn "LI 6")
> backend currently returns 0, causing Hoist to bail and elide GCSE.
>
> Note that constants requiring more than 1 insns to setup were working
> fine since riscv_rtx_costs () was returning non-zero (although that
> itself might need refining: see bugzilla 111139).
>
> To keep testsuite parity, some V tests need updating which started failing
> in the new costing regime.
>
> gcc/ChangeLog:
> * gcc/config/riscv.cc (riscv_rtx_costs): Adjust const_int cost.
> Add some comments about different constants handling.
>
> gcc/testsuite/ChangeLog:
> * gcc.target/riscv/gcse-const.c: New Test
> * gcc.target/riscv/rvv/vsetvl/vlmax_conflict-7.c: Remove test
> for Jump.
> * gcc.target/riscv/rvv/vsetvl/vlmax_conflict-8.c: Ditto.
OK.
jeff
^ permalink raw reply [flat|nested] 3+ messages in thread
* [Committed] RISC-V: Enable Hoist to GCSE simple constants
2023-08-25 18:50 ` Jeff Law
@ 2023-08-25 19:24 ` Vineet Gupta
0 siblings, 0 replies; 3+ messages in thread
From: Vineet Gupta @ 2023-08-25 19:24 UTC (permalink / raw)
To: gcc-patches; +Cc: Jeff Law, gnu-toolchain, Vineet Gupta
Hoist want_to_gcse_p () calls rtx_cost () to compute max distance for
hoist candidates. For a simple const (say 6 which needs seperate insn "LI 6")
backend currently returns 0, causing Hoist to bail and elide GCSE.
Note that constants requiring more than 1 insns to setup were working
fine since riscv_rtx_costs () was returning non-zero (although that
itself might need refining: see bugzilla 111139).
To keep testsuite parity, some V tests need updating which started failing
in the new costing regime.
gcc/ChangeLog:
* config/riscv/riscv.cc (riscv_rtx_costs): Adjust const_int
cost. Add some comments about different constants handling.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/gcse-const.c: New Test
* gcc.target/riscv/rvv/vsetvl/vlmax_conflict-7.c: Remove test
for Jump.
* gcc.target/riscv/rvv/vsetvl/vlmax_conflict-8.c: Ditto.
Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
gcc/config/riscv/riscv.cc | 18 +++++++++---------
gcc/testsuite/gcc.target/riscv/gcse-const.c | 13 +++++++++++++
.../riscv/rvv/vsetvl/vlmax_conflict-7.c | 1 -
.../riscv/rvv/vsetvl/vlmax_conflict-8.c | 1 -
4 files changed, 22 insertions(+), 11 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/gcse-const.c
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 13166d19619c..98a46b00ceb5 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -2490,6 +2490,8 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN
switch (GET_CODE (x))
{
case CONST_INT:
+ /* trivial constants checked using OUTER_CODE in case they are
+ encodable in insn itself w/o need for additional insn(s). */
if (riscv_immediate_operand_p (outer_code, INTVAL (x)))
{
*total = 0;
@@ -2507,17 +2509,15 @@ riscv_rtx_costs (rtx x, machine_mode mode, int outer_code, int opno ATTRIBUTE_UN
/* Fall through. */
case CONST:
+ /* Non trivial CONST_INT Fall through: check if need multiple insns. */
if ((cost = riscv_const_insns (x)) > 0)
{
- /* If the constant is likely to be stored in a GPR, SETs of
- single-insn constants are as cheap as register sets; we
- never want to CSE them. */
- if (cost == 1 && outer_code == SET)
- *total = 0;
- /* When we load a constant more than once, it usually is better
- to duplicate the last operation in the sequence than to CSE
- the constant itself. */
- else if (outer_code == SET || GET_MODE (x) == VOIDmode)
+ /* 1. Hoist will GCSE constants only if TOTAL returned is non-zero.
+ 2. For constants loaded more than once, the approach so far has
+ been to duplicate the operation than to CSE the constant.
+ 3. TODO: make cost more accurate specially if riscv_const_insns
+ returns > 1. */
+ if (outer_code == SET || GET_MODE (x) == VOIDmode)
*total = COSTS_N_INSNS (1);
}
else /* The instruction will be fetched from the constant pool. */
diff --git a/gcc/testsuite/gcc.target/riscv/gcse-const.c b/gcc/testsuite/gcc.target/riscv/gcse-const.c
new file mode 100644
index 000000000000..b04707ce9745
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/gcse-const.c
@@ -0,0 +1,13 @@
+/* Slightly modified copy of gcc.target/arm/pr40956.c. */
+/* { dg-options "-Os" } */
+/* Make sure the constant "6" is loaded into register only once. */
+/* { dg-final { scan-assembler-times "\tli.*6" 1 } } */
+
+int foo(int p, int* q)
+{
+ if (p!=9)
+ *q = 6;
+ else
+ *(q+1) = 6;
+ return 3;
+}
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-7.c b/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-7.c
index 60ad108666f8..085ca9db8542 100644
--- a/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-7.c
+++ b/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-7.c
@@ -21,5 +21,4 @@ void f (int32_t * restrict in, int32_t * restrict out, size_t n, size_t cond, si
}
/* { dg-final { scan-assembler-times {vsetvli} 4 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
-/* { dg-final { scan-assembler-times {j\s+\.L[0-9]+\s+\.L[0-9]+:\s+vlm\.v} 1 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
/* { dg-final { scan-assembler-times {vsetvli\s+[a-x0-9]+,\s*zero,\s*e8,\s*m8,\s*t[au],\s*m[au]} 3 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
diff --git a/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-8.c b/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-8.c
index 7b9574cc332d..fbca9b00e54e 100644
--- a/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-8.c
+++ b/gcc/testsuite/gcc.target/riscv/rvv/vsetvl/vlmax_conflict-8.c
@@ -21,7 +21,6 @@ void f (int32_t * restrict in, int32_t * restrict out, size_t n, size_t cond, si
}
/* { dg-final { scan-assembler-times {vsetvli} 5 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
-/* { dg-final { scan-assembler-times {j\s+\.L[0-9]+\s+\.L[0-9]+:\s+vlm\.v} 1 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
/* { dg-final { scan-assembler-times {vsetvli\s+[a-x0-9]+,\s*zero,\s*e8,\s*m8,\s*t[au],\s*m[au]} 3 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
/* { dg-final { scan-assembler-times {vsetvli\s+[a-x0-9]+,\s*zero,\s*e8,\s*mf8,\s*t[au],\s*m[au]} 1 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
/* { dg-final { scan-assembler-times {vsetvli\s+[a-x0-9]+,\s*zero,\s*e16,\s*mf2,\s*t[au],\s*m[au]} 1 { target { no-opts "-O0" no-opts "-O1" no-opts "-Os" no-opts "-Oz" no-opts "-funroll-loops" no-opts "-g" } } } } */
--
2.34.1
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-08-25 19:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-25 5:16 [PATCH v2] RISC-V: Enable Hoist to GCSE simple constants Vineet Gupta
2023-08-25 18:50 ` Jeff Law
2023-08-25 19:24 ` [Committed] " Vineet Gupta
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).