On Fri, May 15, 2026 at 4:25 AM Roger Sayle <[email protected]> wrote: > > > Hi Hongtao, > Many thanks for the review and suggested improvements. > > This revision implements Hongtao's suggestions/fixes to support > TImode values in non-general hard registers, and adds two more > test cases. Alas things turned out to be a little more complicated > than originally proposed; previously STV used PUT_MODE on TImode > pseudo registers to change their mode everywhere, but something > different is required for hard registers, which may be used in > multiple modes in a function. > > To demonstrate the (additional) benefits, consider the function: > > register __int128 x __asm("xmm0"); > register __int128 y __asm("xmm1"); > __int128 m; > > void foo() > { > m = x ^ y; > } > > Previously GCC on x86_64 with -O2 generated: > > foo: movaps %xmm0, -24(%rsp) > movq -24(%rsp), %rax > movq -16(%rsp), %rdx > movaps %xmm1, -24(%rsp) > xorq -24(%rsp), %rax > xorq -16(%rsp), %rdx > movq %rax, m(%rip) > movq %rdx, m+8(%rip) > ret > > With this revised patch, we now generate: > > foo: movdqa %xmm0, %xmm2 > pxor %xmm1, %xmm2 > movaps %xmm2, m(%rip) > ret > > > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap > and make -k check, both with and without --target_board=unix{-m32} > with no new failures. LGTM. > > > 2026-05-14 Roger Sayle <[email protected]> > Hongtao Liu <[email protected]> > > gcc/ChangeLog > * config/i386/i386-features.cc (scalar_chain): If the chain > starts with a register-to-register move from a hard register, > then the hard register's defs don't need to converted. > (timode_scalar_chain::compute_convert_gain): Provide costs > for hard_reg-to-pseudo and pseudo-to-hard_reg moves. > Tweak speed cost of timode_concatdi_p moves. > (timode_scalar_chain::convert_insn): Add support for > hard_reg-to-pseudo and pseudo-to-hard_reg TImode transfers. > (timode_scalar_to_vector_candidate_p): Likewise. > > gcc/testsuite/ChangeLog > * gcc.target/i386/avx-stv-1.c: New test case. > * gcc.target/i386/sse2-stv-3.c: Likewise. > * gcc.target/i386/sse2-stv-4.c: Likewise. > * gcc.target/i386/sse2-stv-5.c: Likewise. > > > Thanks again, > Roger > -- > > > -----Original Message----- > > From: Hongtao Liu <[email protected]> > > Sent: 05 May 2026 02:44 > > To: Roger Sayle <[email protected]> > > Cc: GCC Patches <[email protected]>; Liu, Hongtao > > <[email protected]>; Uros Bizjak <[email protected]> > > Subject: Re: [x86_64 PATCH] Handle hard registers in STV with inter-unit > > moves. > > > > On Tue, May 5, 2026 at 9:30 AM Hongtao Liu <[email protected]> wrote: > > > > > > On Sun, May 3, 2026 at 6:58 AM Roger Sayle <[email protected]> > > wrote: > > > > > > > > > > > > This patch extends the types of chains that can be converted by > > > > x86's TImode Scalar-To-Vector (STV) pass, to include chains that > > > > originate and/or terminate with moves from/to hard registers. > > > > Currently STV candidate instructions explicitly exclude those than > > > > mention hard registers. > > > > > > > > As motivation, consider the four following functions: > > > > > > > > __int128 a, b, c, z; > > > > __int128 fun(); > > > > > > > > void foo_in(__int128 x) { z = (x ^ a ^ b ^ c); } > > > > > > > > __int128 foo_out() { return (z ^ a ^ b ^ c); } > > > > > > > > __int128 foo_inout(__int128 x) { return (x ^ a ^ b ^ c ^ z); } > > > > > > > > void foo_fun() { z = (fun() ^ a ^ b ^ c); } > > > > > > > > Of these, only the first, foo_in, is currently STV converted to use > > > > SSE instructions. Its incoming argument is constructed from a > > > > concat of two DImode registers, and support for this idiom was added > > > > in a previous STV patch. The next two functions aren't converted > > > > because the chain terminates with a return, which places the TImode > > > > result in a hard register. Likewise, the final foo_fun case isn't > > > > converted as the result from fun initiates a chain from a hard register. > > > > > > > > This patch supports STV conversion of TImode register-to-register > > > > moves, where either the source or the destination (but not both) is > > > > a hard register, by implementing it as a (relatively expensive) > > > > inter-unit move. > > > > > > > > Before, with -O2 -mavx: > > > > > > > > foo_out: > > > > movq z(%rip), %rax > > > > movq z+8(%rip), %rdx > > > > xorq a(%rip), %rax > > > > xorq a+8(%rip), %rdx > > > > xorq b(%rip), %rax > > > > xorq b+8(%rip), %rdx > > > > xorq c(%rip), %rax > > > > xorq c+8(%rip), %rdx > > > > ret > > > > > > > > After, with -O2 -mavx: > > > > > > > > foo_out: > > > > vmovdqa z(%rip), %xmm0 > > > > vpxor a(%rip), %xmm0, %xmm0 > > > > vpxor b(%rip), %xmm0, %xmm0 > > > > vpxor c(%rip), %xmm0, %xmm0 > > > > vpextrq $1, %xmm0, %rdx > > > > vmovq %xmm0, %rax > > > > ret > > > > > > > > Likewise for foo_fun, before with -O2 -mavx: > > > > > > > > foo_fun: > > > > subq $8, %rsp > > > > call fun > > > > movq a(%rip), %rsi > > > > movq a+8(%rip), %rdi > > > > xorq b(%rip), %rsi > > > > xorq b+8(%rip), %rdi > > > > xorq c(%rip), %rsi > > > > xorq c+8(%rip), %rdi > > > > xorq %rax, %rsi > > > > xorq %rdx, %rdi > > > > movq %rsi, z(%rip) > > > > movq %rdi, z+8(%rip) > > > > addq $8, %rsp > > > > ret > > > > > > > > After with -O2 -mavx: > > > > > > > > foo_fun: > > > > subq $8, %rsp > > > > call fun > > > > vmovdqa a(%rip), %xmm0 > > > > vpxor b(%rip), %xmm0, %xmm0 > > > > vmovq %rax, %xmm2 > > > > vpxor c(%rip), %xmm0, %xmm0 > > > > vpinsrq $1, %rdx, %xmm2, %xmm1 > > > > vpxor %xmm1, %xmm0, %xmm0 > > > > vmovdqa %xmm0, z(%rip) > > > > addq $8, %rsp > > > > ret > > > > > > > > The one small subtlety in this patch is in the cost calculation for > > > > inter-unit moves, which now correctly uses both sse_to_integer and > > > > integer_to_sse costs. This patch models the transfer of double word > > > > transfers between units as interunit_cost + COSTS_N_INSNS(1), i.e. > > > > that the two transfers are pipelined in parallel, so that the high > > > > latency is accounted for once [rather than 2*interunit_cost that > > > > assumes the transfers take place strictly sequentially with twice > > > > the single word transfer latency]. > > > > > > > > > > > > This patch has been tested on x86_64-pc-linux-gnu with make > > > > bootstrap and make -k check, both with and without > > > > --target_board=unix{-m32} with no new failures. Ok for mainline? > > > > > > > > > > > > 2026-05-02 Roger Sayle <[email protected]> > > > > > > > > gcc/ChangeLog > > > > * config/i386/i386-features.cc (scalar_chain): If the chain > > > > starts with a register-to-register move from a hard register, > > > > then the hard register's defs don't need to converted. > > > > (timode_scalar_chain::compute_convert_gain): Provide costs > > > > for hard_reg-to-pseudo and pseudo-to-hard_reg moves. > > > > Tweak speed cost of timode_concatdi_p moves. > > > > (timode_scalar_chain::convert_insn): Add support for > > > > hard_reg-to-pseudo and pseudo-to-hard_reg TImode transfers. > > > > (timode_scalar_to_vector_candidate_p): Likewise. > > > > > > > > gcc/testsuite/ChangeLog > > > > * gcc.target/i386/avx-stv-1.c: New test case. > > > > * gcc.target/i386/sse2-stv-3.c: Likewise. > > > > > > > > > > > > > >@@ -1961,7 +1991,8 @@ timode_scalar_chain::convert_insn (rtx_insn > > > *insn) > > > > switch (GET_CODE (dst)) > > > > { > > > > case REG: > > > >- if (GET_MODE (dst) == TImode) > > > >+ if (GET_MODE (dst) == TImode > > > >+ && !HARD_REGISTER_P (dst)) > > > I think it should be !GENERAL_REGNO_P (REGNO (dst)), there's will be > > > ICE for > > Just GENERAL_REG_P (dst). > > > > > void foo_out1() { > > > register __int128 x __asm("xmm0"); > > > z = (x ^ a ^ b ^ c); > > > } > > > > > > Similar for the change in compute_convert_gain and below > > > > > > > { > > > > PUT_MODE (dst, V1TImode); > > > > fix_debug_reg_uses (dst); > > > >@@ -1988,8 +2019,40 @@ timode_scalar_chain::convert_insn (rtx_insn > > *insn) > > > > case REG: > > > > if (GET_MODE (src) == TImode) > > > > { > > > >- PUT_MODE (src, V1TImode); > > > >- fix_debug_reg_uses (src); > > > >+ if (HARD_REGISTER_P (src)) > > > Ditto here. > > > >+ { > > > >+ rtx lo = gen_reg_rtx (DImode); > > > >+ rtx hi = gen_reg_rtx (DImode); > > > >+ emit_insn_before (gen_rtx_SET (lo, gen_lowpart (DImode, > > > >src)), > > > >+ insn); > > > >+ emit_insn_before (gen_rtx_SET (hi, gen_highpart (DImode, > > > >src)), > > > >+ insn); > > > >+ src = gen_reg_rtx (V2DImode); > > > >+ emit_insn_before (gen_vec_concatv2di (src, lo, hi), insn); > > > >+ src = gen_lowpart (V1TImode, src); > > > >+ } > > > >+ else > > > >+ { > > > >+ PUT_MODE (src, V1TImode); > > > >+ fix_debug_reg_uses (src); > > > >+ } > > > >+ } > > > >+ if (REG_P (dst) && HARD_REGISTER_P (dst)) > > > Ditto here. > > > > > > >+ { > > > >+ rtx tmp = gen_reg_rtx (V2DImode); > > > >+ src = gen_lowpart (V2DImode, src); > > > >+ emit_insn_before (gen_rtx_SET (tmp, src), insn); > > > >+ /* Extracting hi before lo helps register allocation. */ > > > >+ rtx hi = gen_reg_rtx (DImode); > > > >+ rtx lo = gen_reg_rtx (DImode); > > > >+ emit_insn_before (gen_vec_extractv2didi (hi, tmp, const1_rtx), > > > >insn); > > > >+ emit_insn_before (gen_vec_extractv2didi (lo, tmp, > > > >+ const0_rtx), insn); > > > >+ > > > >+ /* Construct *concatditi3 pattern from lo and hi. */ > > > >+ hi = gen_rtx_ZERO_EXTEND (TImode, hi); > > > >+ hi = gen_rtx_ASHIFT (TImode, hi, GEN_INT (64)); > > > >+ lo = gen_rtx_ZERO_EXTEND (TImode, lo); > > > >+ src = gen_rtx_PLUS (TImode, hi, lo); > > > > } > > > > break; > > > > > > > > > > But for timode_scalar_to_vector_candidate_p, I think we can keep > > > HARD_REGISTER_P, since xmm0 should also be ok. > > > > > > > > Thanks, > > > > Roger > > > > -- > > > > > > > > > > > > > -- > > > BR, > > > Hongtao > > > > > > > > -- > > BR, > > Hongtao
-- BR, Hongtao
