Jeffrey Law <[email protected]> writes:
> On 6/5/2026 7:50 AM, Jakub Jelinek wrote:
>> On Fri, Jun 05, 2026 at 02:06:44PM +0100, Richard Sandiford wrote:
>>> Yeah, agreed. Namely:
>>>
>>> (define_expand "bswapsi2"
>>> [(set (match_operand:SI 0 "register_operand")
>>> (bswap:SI (match_operand:SI 1 "register_operand")))]
>>> "TARGET_ZBB || TARGET_ZBKB || TARGET_XTHEADBB"
>>> {
>>> /* Expose bswapsi2 on TARGET_64BIT so that the gimple store
>>> merging pass will create suitable bswap insns. We can actually
>>> just FAIL that case when generating RTL and let the generic code
>>> handle it. */
>>> if (TARGET_64BIT && !TARGET_XTHEADBB)
>>> FAIL;
>>> })
>>>
>>> I don't think that's a good enough justification for an exception
>>> like this. Even a target hook that says "is bswap cheap?" would IMO
>>> be better than the above (and could default to "is bswap implemented?").
>>> But hopefully the hook could be generalised a bit. For example, maybe
>>> the hook could be a wrapper around:
>>>
>>> /* Return true if we can implement OP for mode MODE directly, without
>>> resorting
>>> to a libfunc. This usually means that OP will be implemented inline.
>>>
>>> Note that this function cannot tell whether the target pattern chooses
>>> to
>>> use libfuncs internally. */
>>>
>>> bool
>>> can_open_code_p (optab op, machine_mode mode)
>>>
>>> and additionally test for "cheapness".
>> In this particular case it is about widen_bswap_or_bitreverse (or e.g. for
>> clz/clrsb also for widen_leading).
>> Now, store merging already has hacks which consider
>> expand_doubleword_bswap_or_bitreverse implementing for 32-bit targets
>> bswapdi2 using 2 bswapsi2 expanders, so I think we can do the same for
>> bswapsi2 using bswapdi2 and right shift.
>>
>> I don't have setup to test this on riscv, but can test on x86_64/i686.
>>
>> 2026-06-05 Jakub Jelinek <[email protected]>
>>
>> * gimple-ssa-store-merging.cc (maybe_optimize_vector_constructor):
>> Also support bswap32 if bswapdi2 expander is present. Add comment
>> about bswap64 support on 32-bit targets with bswapsi2 expander.
>> (pass_optimize_bswap::execute): Likewise.
>> * config/riscv/bitmanip.md (bswapsi2): Change condition to disable
>> the expander on TARGET_64BIT without TARGET_XTHEADBB, never FAIL.
> Mostly OK. THough it does fail pretty miserably on optimize-bswapsi-6.c
> when compiled with -march=rv64gcb (rv64gcb is common these days):
>
>> jlaw@x11-dpi:~/test/obj/rv/gcc/gcc$ make check-gcc
>> RUNTESTFLAGS="--target_board=unix/-march=rv64gcb
>> dg.exp=optimize-bswapsi-6.c"
>> rm -rf testsuite/gcc-parallel
>> make[1]: Entering directory '/home/jlaw/test/obj/rv/gcc/gcc'
>> (rootme=`${PWDCMD-pwd}`; export rootme; \
>> srcdir=`cd /home/jlaw/test/gcc/gcc; ${PWDCMD-pwd}` ; export srcdir ; \
>> if [ -n "" ] \
>> && [ -n "$GCC_RUNTEST_PARALLELIZE_DIR" ] \
>> && [ -f testsuite/gcc-parallel/finished ]; then \
>> rm -rf testsuite/gcc; \
>> else \
>> cd testsuite/gcc; \
>> rm -f tmp-site.exp; \
>> sed '/set tmpdir/ s|testsuite$|testsuite/gcc|' \
>> < ../../site.exp > tmp-site.exp; \
>> /bin/bash ${srcdir}/../move-if-change tmp-site.exp site.exp; \
>> EXPECT=expect ; export EXPECT ; \
>> if [ -f ${rootme}/../expect/expect ] ; then \
>> TCL_LIBRARY=`cd .. ; cd ${srcdir}/../tcl/library ; ${PWDCMD-pwd}` ; \
>> export TCL_LIBRARY ; \
>> fi ; \
>> `if [ -f ${srcdir}/../dejagnu/runtest ] ; then echo
>> ${srcdir}/../dejagnu/runtest ; else echo runtest; fi` --tool gcc
>> --target_board=unix/-march=rv64gcb dg.exp=optimize-bswapsi-6.c; \
>> if [ -n "$GCC_RUNTEST_PARALLELIZE_DIR" ] ; then \
>> touch ${rootme}/testsuite/gcc-parallel/finished; \
>> fi ; \
>> fi )
>> Using /home/jlaw/test/gcc/gcc/testsuite/lib/gcc.exp as tool init file.
>> Test run by jlaw on Fri Jun 5 11:27:04 2026
>> Target is riscv64-unknown-elf
>> Host is x86_64-pc-linux-gnu
>>
>> === gcc tests ===
>>
>> Schedule of variations:
>> unix/-march=rv64gcb
>>
>> Running target unix/-march=rv64gcb
>> Using /usr/share/dejagnu/baseboards/unix.exp as board description file
>> for target.
>> Using /usr/share/dejagnu/config/unix.exp as generic interface file for
>> target.
>> Using /home/jlaw/test/gcc/gcc/testsuite/config/default.exp as
>> tool-and-target-specific interface file.
>> WARNING: Assuming target board is the local machine (which is probably
>> wrong).
>> You may need to set your DEJAGNU environment variable.
>> Running /home/jlaw/test/gcc/gcc/testsuite/gcc.dg/dg.exp ...
>> FAIL: gcc.dg/optimize-bswapsi-6.c scan-tree-dump store-merging "32 bit
>> bswap implementation found at"
>>
>> === gcc Summary ===
>>
>> # of expected passes 1
>> # of unexpected failures 1
>> /home/jlaw/test/obj/rv/gcc/gcc/xgcc version 17.0.0 20260604
>> (experimental) (GCC)
>
> The net difference on the assembly for this case looks like:
>
>> ! rev8 a0,a0
>> ! srai a0,a0,32
>
> Turns into:
>
>> ! srli a5,a0,16
>> ! rev8 a4,a0
>> ! andi a5,a5,255
>> ! srliw a0,a0,24
>> ! slli a5,a5,8
>> ! srli a4,a4,48
>> ! or a5,a5,a0
>> ! slliw a0,a4,16
>> ! or a0,a0,a5
Since this is kind-of my fault for pushing back against Jakub's
original fix, how about the attached? It seems to fix the testcase
above but I've still to check whether that's through fair means or foul.
I'm not sure that the removed builtin_decl_explicit_p (BUILT_IN_BSWAP32)
calls are necessary, since the code would go on to use a BSWAP64
rather than a BSWAP32.
Untested except for the testcase above, but I've cc:ed the patch checker.
Richard
gcc/
* optabs-query.cc (can_open_code_p): Handle more bswap cases,
incorporating logic from...
* gimple-ssa-store-merging.cc (maybe_optimize_vector_constructor)
(pass_optimize_bswap::execute)
(imm_store_chain_info::try_coalesce_bswap): ...here. Use
can_open_code_p instead of direct optab_handler checks.
---
gcc/gimple-ssa-store-merging.cc | 30 ++++++------------------------
gcc/optabs-query.cc | 17 +++++++++++++++++
2 files changed, 23 insertions(+), 24 deletions(-)
diff --git a/gcc/gimple-ssa-store-merging.cc b/gcc/gimple-ssa-store-merging.cc
index 2e5232fa65f..bd16489f30e 100644
--- a/gcc/gimple-ssa-store-merging.cc
+++ b/gcc/gimple-ssa-store-merging.cc
@@ -1570,10 +1570,7 @@ maybe_optimize_vector_constructor (gimple *cur_stmt)
break;
case 32:
if (builtin_decl_explicit_p (BUILT_IN_BSWAP32)
- && (optab_handler (bswap_optab, SImode) != CODE_FOR_nothing
- /* widen_bswap_or_bitreverse can implement 32-bit bswap
- using bswapdi2 and shift. */
- || optab_handler (bswap_optab, DImode) != CODE_FOR_nothing))
+ && can_implement_p (bswap_optab, SImode))
{
load_type = uint32_type_node;
fndecl = builtin_decl_explicit (BUILT_IN_BSWAP32);
@@ -1584,12 +1581,7 @@ maybe_optimize_vector_constructor (gimple *cur_stmt)
break;
case 64:
if (builtin_decl_explicit_p (BUILT_IN_BSWAP64)
- && (optab_handler (bswap_optab, DImode) != CODE_FOR_nothing
- /* expand_doubleword_bswap_or_bitreverse can use 2 bswapsi2
- expanders on 32-bit targets. */
- || (word_mode == SImode
- && builtin_decl_explicit_p (BUILT_IN_BSWAP32)
- && optab_handler (bswap_optab, SImode) != CODE_FOR_nothing)))
+ && can_implement_p (bswap_optab, DImode))
{
load_type = uint64_type_node;
fndecl = builtin_decl_explicit (BUILT_IN_BSWAP64);
@@ -1636,16 +1628,9 @@ pass_optimize_bswap::execute (function *fun)
tree bswap32_type = NULL_TREE, bswap64_type = NULL_TREE;
bswap32_p = (builtin_decl_explicit_p (BUILT_IN_BSWAP32)
- && (optab_handler (bswap_optab, SImode) != CODE_FOR_nothing
- /* widen_bswap_or_bitreverse can implement 32-bit bswap
- using bswapdi2 and shift. */
- || (optab_handler (bswap_optab, DImode)
- != CODE_FOR_nothing)));
+ && can_implement_p (bswap_optab, SImode));
bswap64_p = (builtin_decl_explicit_p (BUILT_IN_BSWAP64)
- && (optab_handler (bswap_optab, DImode) != CODE_FOR_nothing
- /* expand_doubleword_bswap_or_bitreverse can use 2 bswapsi2
- expanders on 32-bit targets. */
- || (bswap32_p && word_mode == SImode)));
+ && can_implement_p (bswap_optab, DImode));
/* Determine the argument type of the builtins. The code later on
assumes that the return and argument type are the same. */
@@ -3145,15 +3130,12 @@ imm_store_chain_info::try_coalesce_bswap
(merged_store_group *merged_store,
break;
case 32:
if (builtin_decl_explicit_p (BUILT_IN_BSWAP32)
- && optab_handler (bswap_optab, SImode) != CODE_FOR_nothing)
+ && can_implement_p (bswap_optab, SImode))
break;
return false;
case 64:
if (builtin_decl_explicit_p (BUILT_IN_BSWAP64)
- && (optab_handler (bswap_optab, DImode) != CODE_FOR_nothing
- || (word_mode == SImode
- && builtin_decl_explicit_p (BUILT_IN_BSWAP32)
- && optab_handler (bswap_optab, SImode) !=
CODE_FOR_nothing)))
+ && can_implement_p (bswap_optab, DImode))
break;
return false;
default:
diff --git a/gcc/optabs-query.cc b/gcc/optabs-query.cc
index e596d8db881..8226356c7d4 100644
--- a/gcc/optabs-query.cc
+++ b/gcc/optabs-query.cc
@@ -840,6 +840,23 @@ can_open_code_p (optab op, machine_mode mode)
&& can_implement_p (op == neg_optab ? xor_optab : and_optab, new_mode))
return true;
+ scalar_int_mode int_mode;
+ if (op == bswap_optab && is_a<scalar_int_mode> (mode, &int_mode))
+ {
+ /* widen_bswap_or_bitreverse can implement smaller bswaps using
+ wider bswaps and a shift. */
+ opt_scalar_int_mode wider_mode_iter;
+ FOR_EACH_WIDER_MODE (wider_mode_iter, int_mode)
+ if (optab_handler (op, wider_mode_iter.require ()) != CODE_FOR_nothing)
+ return true;
+
+ /* expand_doubleword_bswap_or_bitreverse can use 2 word bswaps to
+ implement a doubleword bswap. */
+ if (GET_MODE_SIZE (int_mode) == 2 * UNITS_PER_WORD
+ && optab_handler (op, word_mode) != CODE_FOR_nothing)
+ return true;
+ }
+
return false;
}
--
2.54.0
