| Message ID | 003101d98023$1be6c6e0$53b454a0$@nextmovesoftware.com |
|---|---|
| State | New |
| Headers | show |
| Series | [x86_64] Introduce insvti_highpart define_insn_and_split. | expand |
On Sat, May 6, 2023 at 4:00 PM Roger Sayle <roger@nextmovesoftware.com> wrote: > > > Hi Uros, > This is a repost/respin of a patch that was conditionally approved: > https://gcc.gnu.org/pipermail/gcc-patches/2023-January/609470.html > > This patch adds a convenient post-reload splitter for setting/updating > the highpart of a TImode variable, using i386's previously added > split_double_concat infrastructure. > > For the new test case below: > > __int128 foo(__int128 x, unsigned long long y) > { > __int128 t = (__int128)y << 64; > __int128 r = (x & ~0ull) | t; > return r; > } > > mainline GCC with -O2 currently generates: > > foo: movq %rdi, %rcx > xorl %eax, %eax > xorl %edi, %edi > orq %rcx, %rax > orq %rdi, %rdx > ret > > with this patch, GCC instead now generates the much better: > > foo: movq %rdi, %rcx > movq %rcx, %rax > ret > > It turns out that the -m32 equivalent of this testcase, already > avoids using explict orl/xor instructions, as it gets optimized > (in combine) by a completely different path. Given that this idiom > isn't seen in 32-bit code (so this pattern doesn't match with -m32), > and also that the shorter 32-bit AND bitmask is represented as a > CONST_INT rather than a CONST_WIDE_INT, this new define_insn_and_split > is implemented for just TARGET_64BIT rather than contort a "generic" > implementation using DWI mode iterators. > > 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 now that we're back in stage 1? > > > 2023-05-06 Roger Sayle <roger@nextmovesoftware.com> > > gcc/ChangeLog > * config/i386/i386.md (any_or_plus): Move definition earlier. > (*insvti_highpart_1): New define_insn_and_split to overwrite > (insv) the highpart of a TImode register/memory. > > gcc/testsuite/ChangeLog > * gcc.target/i386/insvti_highpart-1.c: New test case. @@ -3479,6 +3479,31 @@ "mov{b}\t{%h1, %h0|%h0, %h1}" [(set_attr "type" "imov") (set_attr "mode" "QI")]) + +(define_code_iterator any_or_plus [plus ior xor]) Please add a line of vertical space here. +(define_insn_and_split "*insvti_highpart_1" ... + && CONST_WIDE_INT_P (operands[3]) + && CONST_WIDE_INT_NUNITS (operands[3]) == 2 + && CONST_WIDE_INT_ELT (operands[3], 0) == -1 + && CONST_WIDE_INT_ELT (operands[3], 1) == 0" + "#" + "&& reload_completed" + [(clobber (const_int 0))] The above RTX is unreachable, but please use [(const_int 0)] as is the case with similar patterns. OK with the above changes. Thanks, Uros.
diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md index d49f1cd..62cafe7 100644 --- a/gcc/config/i386/i386.md +++ b/gcc/config/i386/i386.md @@ -3479,6 +3479,31 @@ "mov{b}\t{%h1, %h0|%h0, %h1}" [(set_attr "type" "imov") (set_attr "mode" "QI")]) + +(define_code_iterator any_or_plus [plus ior xor]) +(define_insn_and_split "*insvti_highpart_1" + [(set (match_operand:TI 0 "nonimmediate_operand" "=ro,r,r,&r") + (any_or_plus:TI + (and:TI + (match_operand:TI 1 "nonimmediate_operand" "r,m,r,m") + (match_operand:TI 3 "const_scalar_int_operand" "n,n,n,n")) + (ashift:TI + (zero_extend:TI + (match_operand:DI 2 "nonimmediate_operand" "r,r,m,m")) + (const_int 64))))] + "TARGET_64BIT + && CONST_WIDE_INT_P (operands[3]) + && CONST_WIDE_INT_NUNITS (operands[3]) == 2 + && CONST_WIDE_INT_ELT (operands[3], 0) == -1 + && CONST_WIDE_INT_ELT (operands[3], 1) == 0" + "#" + "&& reload_completed" + [(clobber (const_int 0))] +{ + operands[4] = gen_lowpart (DImode, operands[1]); + split_double_concat (TImode, operands[0], operands[4], operands[2]); + DONE; +}) ;; Floating point push instructions. @@ -11573,7 +11598,6 @@ (set_attr "mode" "QI")]) ;; Split DST = (HI<<32)|LO early to minimize register usage. -(define_code_iterator any_or_plus [plus ior xor]) (define_insn_and_split "*concat<mode><dwi>3_1" [(set (match_operand:<DWI> 0 "nonimmediate_operand" "=ro,r") (any_or_plus:<DWI> diff --git a/gcc/testsuite/gcc.target/i386/insvti_highpart-1.c b/gcc/testsuite/gcc.target/i386/insvti_highpart-1.c new file mode 100644 index 0000000..4ae9ccf --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/insvti_highpart-1.c @@ -0,0 +1,12 @@ +/* { dg-do compile { target int128 } } */ +/* { dg-options "-O2" } */ + +__int128 foo(__int128 x, unsigned long long y) +{ + __int128 t = (__int128)y << 64; + __int128 r = (x & ~0ull) | t; + return r; +} + +/* { dg-final { scan-assembler-not "xorl" } } */ +/* { dg-final { scan-assembler-not "orq" } } */
Hi Uros, This is a repost/respin of a patch that was conditionally approved: https://gcc.gnu.org/pipermail/gcc-patches/2023-January/609470.html This patch adds a convenient post-reload splitter for setting/updating the highpart of a TImode variable, using i386's previously added split_double_concat infrastructure. For the new test case below: __int128 foo(__int128 x, unsigned long long y) { __int128 t = (__int128)y << 64; __int128 r = (x & ~0ull) | t; return r; } mainline GCC with -O2 currently generates: foo: movq %rdi, %rcx xorl %eax, %eax xorl %edi, %edi orq %rcx, %rax orq %rdi, %rdx ret with this patch, GCC instead now generates the much better: foo: movq %rdi, %rcx movq %rcx, %rax ret It turns out that the -m32 equivalent of this testcase, already avoids using explict orl/xor instructions, as it gets optimized (in combine) by a completely different path. Given that this idiom isn't seen in 32-bit code (so this pattern doesn't match with -m32), and also that the shorter 32-bit AND bitmask is represented as a CONST_INT rather than a CONST_WIDE_INT, this new define_insn_and_split is implemented for just TARGET_64BIT rather than contort a "generic" implementation using DWI mode iterators. 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 now that we're back in stage 1? 2023-05-06 Roger Sayle <roger@nextmovesoftware.com> gcc/ChangeLog * config/i386/i386.md (any_or_plus): Move definition earlier. (*insvti_highpart_1): New define_insn_and_split to overwrite (insv) the highpart of a TImode register/memory. gcc/testsuite/ChangeLog * gcc.target/i386/insvti_highpart-1.c: New test case. Thanks again, Roger --