Re: [x86_64 PATCH] Introduce insvti_highpart define_insn_and_split.

[Uros Bizjak <ubizjak@gmail.com>]

On Thu, Jan 5, 2023 at 3:10 PM Roger Sayle <roger@nextmovesoftware.com> wrote:
>
>
> This patch adds a convenient post-reload splitter for setting/updating
> the highpart of a TImode variable, using the recently 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
>
> [which interestingly shows the deeper (ABI) issue that I'm trying
> to fix, this new define_insn_and_split being the next piece].
>
> 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 (and therefore this pattern wouldn't match),
> 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?  Please let me know if you'd
> prefer a different pattern name [insv seemed better than mov].
>
>
> 2023-01-05  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.

LGTM, but the new pattern looks complex enough to make me a bit
nervous at this development stage. If the patch just extended the
existing SI/DI mode pattern to also handle DI/TI case for 64-bit
targets, I would approve it without hesitation, but since 128 bit
types are not that common ATM, I'd rather postpone introduction of
complex new pattern to the next stage 1.

Target specific part of the compiler does have some more freedom to
introduce new functionality during "general bugfixing" development
stage, so simple patches that extend existing patterns to handle new
modes or constraints should be OK, but let's not stretch this rule too
much.

To follow with the example, your recent extendditi2 splitter patch was
OK even at this development stage, because it just extended existing
patterns and existing approach to also handle 64bit instructions (that
are actually same as 32bit ones).

Uros.