* [RFA][RISC-V] [PATCH v2] Enable inlining str* by default
@ 2024-05-04 14:41 Jeff Law
2024-05-07 15:33 ` Jeff Law
0 siblings, 1 reply; 2+ messages in thread
From: Jeff Law @ 2024-05-04 14:41 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]
The CI system caught a latent bug in the inline string comparison code
that shows up with rv32+zbb. It was hardcoding 64 when AFAICT it should
have been using BITS_PER_WORD.
So v2 with that fixed.
--
So with Chrstoph's patches from late 2022 we've had the ability to
inline strlen, and str[n]cmp (scalar). However, we never actually
turned this capability on by default!
This patch flips the those default to allow inlinining by default. It
also fixes one bug exposed by our internal testing when NBYTES is zero
for strncmp. I don't think that case happens enough to try and optimize
it, we just disable inline expansion for that instance.
This has been bootstrapped and regression tested on rv64gc at various
times as well as cross tested on rv64gc more times than I can probably
count (we've have this patch internally for a while). More importantly,
I just successfully tested it on rv64gc and rv32gcv elf configurations
with the trunk ;-)
OK for the trunk (after passing pre-commit CI)?
Jeff
[-- Attachment #2: P --]
[-- Type: text/plain, Size: 3856 bytes --]
[RFA][RISC-V] Enable inlining str* by default
So with Chrstoph's patches from late 2022 we've had the ability to inline
strlen, and str[n]cmp (scalar). However, we never actually turned this
capability on by default!
This patch flips the those default to allow inlinining by default. It also
fixes one bug exposed by our internal testing when NBYTES is zero for strncmp.
I don't think that case happens enough to try and optimize it, we just disable
inline expansion for that instance.
This has been bootstrapped and regression tested on rv64gc at various times as
well as cross tested on rv64gc more times than I can probably count (we've have
this patch internally for a while). More importantly, I just successfully
tested it on rv64gc and rv32gcv elf configurations with the trunk ;-)
OK for the trunk (assuming it passes pre-commit CI)?
gcc/
* config/riscv/riscv-string.cc (riscv_expand_strcmp): Do not inline
strncmp with zero size.
(emit_strcmp_scalar_compare_subword): Adjust rotation for rv32 vs rv64.
* config/riscv/riscv.opt (var_inline_strcmp): Enable by default.
(vriscv_inline_strncmp, riscv_inline_strlen): Likewise.
gcc/testsuite
* gcc.target/riscv/zbb-strlen-disabled-2.c: Turn off inlining.
diff --git a/gcc/config/riscv/riscv-string.cc b/gcc/config/riscv/riscv-string.cc
index b09b51d7526..41cb061c746 100644
--- a/gcc/config/riscv/riscv-string.cc
+++ b/gcc/config/riscv/riscv-string.cc
@@ -153,7 +153,7 @@ emit_strcmp_scalar_compare_subword (rtx data1, rtx data2, rtx orc1,
rtx imask = gen_rtx_CONST_INT (Xmode, im);
rtx m_reg = gen_reg_rtx (Xmode);
emit_insn (gen_rtx_SET (m_reg, imask));
- do_rotr3 (m_reg, m_reg, GEN_INT (64 - cmp_bytes * BITS_PER_UNIT));
+ do_rotr3 (m_reg, m_reg, GEN_INT (BITS_PER_WORD - cmp_bytes * BITS_PER_UNIT));
do_and3 (data1, m_reg, data1);
do_and3 (data2, m_reg, data2);
if (TARGET_ZBB)
@@ -497,6 +497,13 @@ riscv_expand_strcmp (rtx result, rtx src1, rtx src2,
return false;
nbytes = UINTVAL (bytes_rtx);
+ /* If NBYTES is zero the result of strncmp will always be zero,
+ but that would require special casing in the caller. So for
+ now just don't do an inline expansion. This probably rarely
+ happens in practice, but it is tested by the testsuite. */
+ if (nbytes == 0)
+ return false;
+
/* We don't emit parts of a strncmp() call. */
if (nbytes > compare_max)
return false;
diff --git a/gcc/config/riscv/riscv.opt b/gcc/config/riscv/riscv.opt
index ee824756381..95165e5fa89 100644
--- a/gcc/config/riscv/riscv.opt
+++ b/gcc/config/riscv/riscv.opt
@@ -515,15 +515,15 @@ Target Var(TARGET_INLINE_SUBWORD_ATOMIC) Init(1)
Always inline subword atomic operations.
minline-strcmp
-Target Var(riscv_inline_strcmp) Init(0)
+Target Var(riscv_inline_strcmp) Init(1)
Inline strcmp calls if possible.
minline-strncmp
-Target Var(riscv_inline_strncmp) Init(0)
+Target Var(riscv_inline_strncmp) Init(1)
Inline strncmp calls if possible.
minline-strlen
-Target Var(riscv_inline_strlen) Init(0)
+Target Var(riscv_inline_strlen) Init(1)
Inline strlen calls if possible.
-param=riscv-strcmp-inline-limit=
diff --git a/gcc/testsuite/gcc.target/riscv/zbb-strlen-disabled-2.c b/gcc/testsuite/gcc.target/riscv/zbb-strlen-disabled-2.c
index a481068aa0c..1295aeb0086 100644
--- a/gcc/testsuite/gcc.target/riscv/zbb-strlen-disabled-2.c
+++ b/gcc/testsuite/gcc.target/riscv/zbb-strlen-disabled-2.c
@@ -1,6 +1,6 @@
/* { dg-do compile } */
-/* { dg-options "-march=rv32gc_zbb" { target { rv32 } } } */
-/* { dg-options "-march=rv64gc_zbb" { target { rv64 } } } */
+/* { dg-options "-mno-inline-strlen -march=rv32gc_zbb" { target { rv32 } } } */
+/* { dg-options "-mno-inline-strlen -march=rv64gc_zbb" { target { rv64 } } } */
/* { dg-skip-if "" { *-*-* } { "-O0" "-Os" "-Og" "-Oz" } } */
typedef long unsigned int size_t;
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [RFA][RISC-V] [PATCH v2] Enable inlining str* by default
2024-05-04 14:41 [RFA][RISC-V] [PATCH v2] Enable inlining str* by default Jeff Law
@ 2024-05-07 15:33 ` Jeff Law
0 siblings, 0 replies; 2+ messages in thread
From: Jeff Law @ 2024-05-07 15:33 UTC (permalink / raw)
To: gcc-patches
On 5/4/24 8:41 AM, Jeff Law wrote:
> The CI system caught a latent bug in the inline string comparison code
> that shows up with rv32+zbb. It was hardcoding 64 when AFAICT it should
> have been using BITS_PER_WORD.
>
> So v2 with that fixed.
So per the discussion in today's call I reviewed a couple of spaces,
particularly -Os and interactions with vector expansion of these routines.
WRT vector expansion. We *always* use loops for this stuff right now
(str[n]cmp, strlen). Vector expansion of these routines is suppressed
with -Os enabled, which is good as it's hard to see how the vector loops
will ever be smaller than a function call.
WRT scalar expansion. -Os generally turns off scalar expansion as well,
with the exception of trivial cases involving str[n]cmp with one arg
being a constant string.
These shouldn't interact at all with Sergei's setmem, clrmem, movmem
expanders.
If we look to improve the vector expansion case (say by handling cases
with small counts for strncmp or when one argument to str[n]cmp is a
constant string) in the future, we'll have to revisit.
Overall conclusion is we should go ahead with the patch.
jeff
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2024-05-07 15:33 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-04 14:41 [RFA][RISC-V] [PATCH v2] Enable inlining str* by default Jeff Law
2024-05-07 15:33 ` Jeff Law
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).