From: "Maciej W. Rozycki" <macro@embecosm.com>
To: gcc-patches@gcc.gnu.org
Cc: Andrew Waterman <andrew@sifive.com>,
Jim Wilson <jim.wilson.gcc@gmail.com>,
Kito Cheng <kito.cheng@gmail.com>,
Palmer Dabbelt <palmer@dabbelt.com>
Subject: [PATCH] RISC-V: Also handle sign extension in branch costing
Date: Mon, 8 Jan 2024 00:06:20 +0000 (GMT) [thread overview]
Message-ID: <alpine.DEB.2.20.2401072322250.5892@tpp.orcam.me.uk> (raw)
Complement commit c1e8cb3d9f94 ("RISC-V: Rework branch costing model for
if-conversion") and also handle extraneous sign extend operations that
are sometimes produced by `noce_try_cmove_arith' instead of zero extend
operations, making branch costing consistent. It is unclear what the
condition is for the middle end to choose between the zero extend and
sign extend operation, but the test case included uses sign extension
with 64-bit targets, preventing if-conversion from triggering across all
the architectural variants.
There are further anomalies revealed by the test case, specifically the
exceedingly high branch cost of 6 required for the `-mmovcc' variant
despite that the final branchless sequence only uses 4 instructions, the
missed conversion at -O1 for 32-bit targets even though code is machine
word size agnostic, and the missed conversion at -Os and -Oz for 32-bit
Zicond targets even though the branchless sequence would be shorter than
the branched one. These will have to be handled separately.
gcc/
* config/riscv/riscv.cc (riscv_noce_conversion_profitable_p):
Also handle sign extension.
gcc/testsuite/
* gcc.target/riscv/cset-sext-sfb.c: New test.
* gcc.target/riscv/cset-sext-thead.c: New test.
* gcc.target/riscv/cset-sext-ventana.c: New test.
* gcc.target/riscv/cset-sext-zicond.c: New test.
* gcc.target/riscv/cset-sext.c: New test.
---
Hi,
This is still in regression-testing, but as a branch costing adjustment
only I don't expect any code correctness issues, and the performance
advantage seems very obvious as the sign extend operation applied to the
result of a conditional set instruction is always a no-op, just as with
the zero extension.
Depending on how you look at it you may qualify this as a bug fix (for
the commit referred; it's surely rare enough a case I missed in original
testing) or a missed optimisation. Either way it's a narrow-scoped very
small change, almost an obviously correct one. I'll be very happy to get
it off my plate now, but if it has to wait for GCC 15, I'll accept the
decision.
OK to apply then or shall I wait?
Maciej
---
gcc/config/riscv/riscv.cc | 5 ++-
gcc/testsuite/gcc.target/riscv/cset-sext-sfb.c | 28 +++++++++++++++++++++
gcc/testsuite/gcc.target/riscv/cset-sext-thead.c | 26 +++++++++++++++++++
gcc/testsuite/gcc.target/riscv/cset-sext-ventana.c | 26 +++++++++++++++++++
gcc/testsuite/gcc.target/riscv/cset-sext-zicond.c | 26 +++++++++++++++++++
gcc/testsuite/gcc.target/riscv/cset-sext.c | 27 ++++++++++++++++++++
6 files changed, 136 insertions(+), 2 deletions(-)
gcc-riscv-noce-conversion-profitable-p-sign-extend.diff
Index: gcc/gcc/config/riscv/riscv.cc
===================================================================
--- gcc.orig/gcc/config/riscv/riscv.cc
+++ gcc/gcc/config/riscv/riscv.cc
@@ -3555,7 +3555,7 @@ riscv_noce_conversion_profitable_p (rtx_
this redundant zero extend operation counts towards the cost of
the replacement sequence. Compensate for that by incrementing the
cost of the original sequence as well as the maximum sequence cost
- accordingly. */
+ accordingly. Likewise for sign extension. */
rtx last_dest = NULL_RTX;
for (rtx_insn *insn = seq; insn; insn = NEXT_INSN (insn))
{
@@ -3567,8 +3567,9 @@ riscv_noce_conversion_profitable_p (rtx_
&& GET_CODE (x) == SET)
{
rtx src = SET_SRC (x);
+ enum rtx_code code = GET_CODE (src);
if (last_dest != NULL_RTX
- && GET_CODE (src) == ZERO_EXTEND
+ && (code == SIGN_EXTEND || code == ZERO_EXTEND)
&& REG_P (XEXP (src, 0))
&& REGNO (XEXP (src, 0)) == REGNO (last_dest))
{
Index: gcc/gcc/testsuite/gcc.target/riscv/cset-sext-sfb.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/riscv/cset-sext-sfb.c
@@ -0,0 +1,28 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */
+/* { dg-options "-march=rv32gc -mtune=sifive-7-series -mbranch-cost=1 -fdump-rtl-ce1" { target { rv32 } } } */
+/* { dg-options "-march=rv64gc -mtune=sifive-7-series -mbranch-cost=1 -fdump-rtl-ce1" { target { rv64 } } } */
+
+int
+foo (long a, long b)
+{
+ if (!b)
+ return 0;
+ else if (a)
+ return 1;
+ else
+ return 0;
+}
+
+/* Expect short forward branch assembly like:
+
+ snez a0,a0
+ bne a1,zero,1f # movcc
+ mv a0,zero
+1:
+ */
+
+/* { dg-final { scan-rtl-dump-times "if-conversion succeeded through noce_try_cmove_arith" 1 "ce1" { xfail { rv32 && { any-opts "-O1" } } } } } */
+/* { dg-final { scan-assembler-times "\\ssnez\\s" 1 } } */
+/* { dg-final { scan-assembler-times "\\sbne\\s\[^\\s\]+\\s# movcc\\s" 1 { xfail { rv32 && { any-opts "-O1" } } } } } */
+/* { dg-final { scan-assembler-not "\\sbeq\\s" { xfail { rv32 && { any-opts "-O1" } } } } } */
Index: gcc/gcc/testsuite/gcc.target/riscv/cset-sext-thead.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/riscv/cset-sext-thead.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target rv64 } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */
+/* { dg-options "-march=rv64gc_xtheadcondmov -mtune=thead-c906 -mbranch-cost=1 -fdump-rtl-ce1" } */
+
+int
+foo (long a, long b)
+{
+ if (!b)
+ return 0;
+ else if (a)
+ return 1;
+ else
+ return 0;
+}
+
+/* Expect branchless assembly like:
+
+ snez a0,a0
+ th.mveqz a0,zero,a1
+ */
+
+/* { dg-final { scan-rtl-dump-times "if-conversion succeeded through noce_try_cmove_arith" 1 "ce1" } } */
+/* { dg-final { scan-assembler-times "\\ssnez\\s" 1 } } */
+/* { dg-final { scan-assembler-times "\\s(?:th\\.mveqz|th\\.mvnez)\\s" 1 } } */
+/* { dg-final { scan-assembler-not "\\s(?:beq|bne)\\s" } } */
Index: gcc/gcc/testsuite/gcc.target/riscv/cset-sext-ventana.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/riscv/cset-sext-ventana.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target rv64 } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */
+/* { dg-options "-march=rv64gc_xventanacondops -mtune=rocket -mbranch-cost=3 -fdump-rtl-ce1" } */
+
+int
+foo (long a, long b)
+{
+ if (!b)
+ return 0;
+ else if (a)
+ return 1;
+ else
+ return 0;
+}
+
+/* Expect branchless assembly like:
+
+ snez a0,a0
+ vt.maskc a0,a0,a1
+ */
+
+/* { dg-final { scan-rtl-dump-times "if-conversion succeeded through noce_try_cmove_arith" 1 "ce1" } } */
+/* { dg-final { scan-assembler-times "\\ssnez\\s" 1 } } */
+/* { dg-final { scan-assembler-times "\\svt\\.maskc\\s" 1 } } */
+/* { dg-final { scan-assembler-not "\\s(?:beq|bne)\\s" } } */
Index: gcc/gcc/testsuite/gcc.target/riscv/cset-sext-zicond.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/riscv/cset-sext-zicond.c
@@ -0,0 +1,26 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" } } */
+/* { dg-options "-march=rv64gc_zicond -mtune=rocket -mbranch-cost=3 -fdump-rtl-ce1" { target { rv64 } } } */
+/* { dg-options "-march=rv32gc_zicond -mtune=rocket -mbranch-cost=3 -fdump-rtl-ce1" { target { rv32 } } } */
+
+int
+foo (long a, long b)
+{
+ if (!b)
+ return 0;
+ else if (a)
+ return 1;
+ else
+ return 0;
+}
+
+/* Expect branchless assembly like:
+
+ snez a0,a0
+ czero.eqz a0,a0,a1
+ */
+
+/* { dg-final { scan-rtl-dump-times "if-conversion succeeded through noce_try_cmove_arith" 1 "ce1" { xfail { rv32 && { any-opts "-O1" "-Os" "-Oz" } } } } } */
+/* { dg-final { scan-assembler-times "\\ssnez\\s" 1 } } */
+/* { dg-final { scan-assembler-times "\\sczero\\.eqz\\s" 1 { xfail { rv32 && { any-opts "-O1" "-Os" "-Oz" } } } } } */
+/* { dg-final { scan-assembler-not "\\s(?:beq|bne)\\s" { xfail { rv32 && { any-opts "-O1" "-Os" "-Oz" } } } } } */
Index: gcc/gcc/testsuite/gcc.target/riscv/cset-sext.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/riscv/cset-sext.c
@@ -0,0 +1,27 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-O0" "-Og" "-Os" "-Oz" } } */
+/* { dg-options "-march=rv32gc -mtune=sifive-5-series -mbranch-cost=6 -mmovcc -fdump-rtl-ce1" { target { rv32 } } } */
+/* { dg-options "-march=rv64gc -mtune=sifive-5-series -mbranch-cost=6 -mmovcc -fdump-rtl-ce1" { target { rv64 } } } */
+
+int
+foo (long a, long b)
+{
+ if (!b)
+ return 0;
+ else if (a)
+ return 1;
+ else
+ return 0;
+}
+
+/* Expect branchless assembly like:
+
+ snez a1,a1
+ neg a1,a1
+ snez a0,a0
+ and a0,a1,a0
+ */
+
+/* { dg-final { scan-rtl-dump-times "if-conversion succeeded through noce_try_cmove_arith" 1 "ce1" { xfail { rv32 && { any-opts "-O1" } } } } } */
+/* { dg-final { scan-assembler-times "\\ssnez\\s" 2 { xfail { rv32 && { any-opts "-O1" } } } } } */
+/* { dg-final { scan-assembler-not "\\s(?:beq|bne)\\s" { xfail { rv32 && { any-opts "-O1" } } } } } */
next reply other threads:[~2024-01-08 0:06 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-08 0:06 Maciej W. Rozycki [this message]
2024-01-09 14:43 ` Jeff Law
2024-01-10 16:40 ` Maciej W. Rozycki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.20.2401072322250.5892@tpp.orcam.me.uk \
--to=macro@embecosm.com \
--cc=andrew@sifive.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jim.wilson.gcc@gmail.com \
--cc=kito.cheng@gmail.com \
--cc=palmer@dabbelt.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).