[Bug target/105066] New: GCC thinks pinsrw xmm, mem, 0 requires SSE4.1, not SSE2? _mm_loadu_si16 bounces through integer reg

[Bug target/105066] New: GCC thinks pinsrw xmm, mem, 0 requires SSE4.1, not SSE2? _mm_loadu_si16 bounces through integer reg

[peter at cordes dot ca]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105066

            Bug ID: 105066
           Summary: GCC thinks pinsrw xmm, mem, 0 requires SSE4.1, not
                    SSE2?  _mm_loadu_si16 bounces through integer reg
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Keywords: missed-optimization
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: peter at cordes dot ca
  Target Milestone: ---
            Target: x86_64-*-*, i?86-*-*

PR99754 fixed the wrong-code for _mm_loadu_si16, but the resulting asm is not
efficient without -msse4.1 (as part of -march= most things).  It seems GCC
thinks that pinsrw / pextrw with a memory operand requires SSE4.1, like
pinsr/extr for b/d/q operand-size.  But actually 16-bit insr/extr only needs
SSE2

(We're also not efficiently folding it into a memory source operand for
PMOVZXBQ, see below)

https://godbolt.org/z/dYchb6hec shows GCC trunk 12.0.1 20220321

__m128i load16(void *p){
    return _mm_loadu_si16( p );
}


load16(void*):     # no options, or -march=core2 or -mssse3
        movzwl  (%rdi), %eax
        pxor    %xmm1, %xmm1
        pinsrw  $0, %eax, %xmm1   # should be MOVD %eax, or PINSRW mem
        movdqa  %xmm1, %xmm0
        ret

vs. 

load16(void*):      # -msse4.1
        pxor    %xmm1, %xmm1
        pinsrw  $0, (%rdi), %xmm1
        movdqa  %xmm1, %xmm0
        ret


The second version is actually 100% fine with SSE2:
https://www.felixcloutier.com/x86/pinsrw shows that there's only a single
opcode for PINSRW xmm, r32/m16, imm8 and it requires SSE2; reg vs. mem source
is just a matter of the modr/m byte.

The same problem exists for _mm_storeu_si16 not using pextrw to memory (which
is also SSE2), instead bouncing through EAX.  (Insanely still PEXTRW instead of
MOVD).

----

There is a choice of strategy here, but pinsrw/extrw between eax and xmm0 is
clearly sub-optimal everywhere.  Once we factor out the dumb register
allocation that wastes a movdqa, the interesting options are:

    movzwl  (%rdi), %eax      # 1 uop on everything
    movd    %eax, %xmm0       # 1 uop on everything

vs.

    pxor    %xmm0, %xmm0        # 1 uop for the front-end, eliminated on Intel
    pinsrw  $0, (%rdi), %xmm0   # 2 uops  (load + shuffle/merge)


Similarly for extract,

    pextrw  $0, %xmm0, (%rdi)   # 2 uops on most

vs.

    movd    %xmm0, %eax         # 1 uop, only 1/clock even on Ice Lake
    movw    %ax, (%rdi)         # 1 uop

On Bulldozer-family, bouncing through an integer reg adds a lot of latency vs.
loading straight into the SIMD unit.  (2 integer cores share a SIMD/FP unit, so
movd between XMM and GP-integer is higher latency than most.)  So that would
definitely favour pinsrw/pextrw with memory.

On Ice Lake, pextrw to mem is 2/clock throughput: the SIMD shuffle can run on
p1/p5.  But MOVD r,v is still p0 only, and MOVD v,r is still p5 only.  So that
also favours pinsrw/pextrw with memory, despite the extra front-end uop for
pxor-zeroing the destination on load.

Of course, if _mm_storeu_si16 is used on a temporary that's later reloaded,
being able to optimize to a movd (and optionally movzx) is very good.  Similar
for _mm_loadu_si16 on a value we have in an integer reg, especially if we know
it's already zero-extended to 32-bit for just a movd, we'd like to be able to
do that.

---

It's also essential that these loads fold efficiently into memory source
operands for PMOVZX; pmovzxbq is one of the major use-cases for a 16-bit load.

That may be a separate bug, IDK

https://godbolt.org/z/3a9T55n3q shows _mm_cvtepu8_epi32(_mm_loadu_si32(p)) does
fold a 32-bit memory source operand nicely to pmovzxbd (%rdi), %xmm0 which can
micro-fuse into a single uop on Intel CPUs (for the 128-bit destination
version, not YMM), but disaster with 16-bit loads:

__m128i pmovzxbq(void *p){
    return _mm_cvtepu8_epi64(_mm_loadu_si16(p));
}

pmovzxbq(void*):  # -O3 -msse4.1 -mtune=haswell
        pxor    %xmm0, %xmm0                  # 1 uop
        pinsrw  $0, (%rdi), %xmm0             # 2 uops, one for shuffle port
        pmovzxbq        %xmm0, %xmm0          # 1 uop for the same shuffle port
        ret

(_mm_cvtepu8_epi64 requires SSE4.1 so there's no interaction with the
-mno-sse4.1 implementation of the load.)

[Bug target/105066] GCC thinks pinsrw xmm, mem, 0 requires SSE4.1, not SSE2? _mm_loadu_si16 bounces through integer reg

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105066

--- Comment #1 from Hongtao.liu <crazylht at gmail dot com> ---
pinsrw is under sse2 for both reg and mem operands, but not for pextrw which
requires sse4.1 for memory operands.

10593(define_insn "vec_set<mode>_0"
10594  [(set (match_operand:V8_128 0 "register_operand"
10595          "=v,v,v,x,x,Yr,*x,x,x,x,v,v")
10596        (vec_merge:V8_128
10597          (vec_duplicate:V8_128
10598            (match_operand:<ssescalarmode> 2 "nonimmediate_operand"
10599          " r,m,v,r,m,Yr,*x,r,m,x,r,m"))
10600          (match_operand:V8_128 1 "reg_or_0_operand"
10601          " C,C,v,0,0,0 ,0 ,x,x,x,v,v")
10602          (const_int 1)))]
10603  "TARGET_SSE2"
10604  "@
10605   vmovw\t{%k2, %0|%0, %k2}
10606   vmovw\t{%2, %0|%0, %2}
10607   vmovsh\t{%2, %1, %0|%0, %1, %2}
10608   pinsrw\t{$0, %k2, %0|%0, %k2, 0}
10609   pinsrw\t{$0, %2, %0|%0, %2, 0}
10610   pblendw\t{$1, %2, %0|%0, %2, 1}
10611   pblendw\t{$1, %2, %0|%0, %2, 1}
10612   vpinsrw\t{$0, %k2, %1, %0|%0, %1, %k2, 0}
10613   vpinsrw\t{$0, %2, %1, %0|%0, %1, %2, 0}
10614   vpblendw\t{$1, %2, %1, %0|%0, %1, %2, 1}
10615   vpinsrw\t{$0, %k2, %1, %0|%0, %1, %k2, 0}
10616   vpinsrw\t{$0, %2, %1, %0|%0, %1, %2, 0}"
10617  [(set (attr "isa")
10618        (cond [(eq_attr "alternative" "0,1,2")
10619                 (const_string "avx512fp16")
10620               (eq_attr "alternative" "3")
10621                 (const_string "noavx")
10622               (eq_attr "alternative" "4,5,6")
10623                 (const_string "sse4_noavx")

alternative 4 doesn't require sse4.


and for performance pinsw mem > vmovd reg > pinsrw reg

and yes, it's sub-optimization for below.

pmovzxbq(void*):  # -O3 -msse4.1 -mtune=haswell
        pxor    %xmm0, %xmm0                  # 1 uop
        pinsrw  $0, (%rdi), %xmm0             # 2 uops, one for shuffle port
        pmovzxbq        %xmm0, %xmm0          # 1 uop for the same shuffle port
        ret

[Bug target/105066] GCC thinks pinsrw xmm, mem, 0 requires SSE4.1, not SSE2? _mm_loadu_si16 bounces through integer reg

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105066

--- Comment #2 from Hongtao.liu <crazylht at gmail dot com> ---

> That may be a separate bug, IDK
> 

Open PR105072 for it.

[Bug target/105066] GCC thinks pinsrw xmm, mem, 0 requires SSE4.1, not SSE2? _mm_loadu_si16 bounces through integer reg

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105066

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2022-03-28
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW

[Bug target/105066] GCC thinks pinsrw xmm, mem, 0 requires SSE4.1, not SSE2? _mm_loadu_si16 bounces through integer reg

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105066

--- Comment #3 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by hongtao Liu <liuhongt@gcc.gnu.org>:

https://gcc.gnu.org/g:e4352a0fee49441a32d12e8d8b98c425cfed4a86

commit r12-7841-ge4352a0fee49441a32d12e8d8b98c425cfed4a86
Author: liuhongt <hongtao.liu@intel.com>
Date:   Mon Mar 28 11:12:37 2022 +0800

    Fix typo in vec_setv8hi_0.

    pinsrw is available for both reg and mem operand under sse2.
    pextrw requires sse4.1 for mem operands.

    The patch change attr "isa" for pinsrw mem alternative from sse4_noavx
    to noavx, will enable below optimization.

    -        movzwl  (%rdi), %eax
             pxor    %xmm1, %xmm1
    -        pinsrw  $0, %eax, %xmm1
    +        pinsrw  $0, (%rdi), %xmm1
             movdqa  %xmm1, %xmm0

    gcc/ChangeLog:

            PR target/105066
            * config/i386/sse.md (vec_set<mode>_0): Change attr "isa" of
            alternative 4 from sse4_noavx to noavx.

    gcc/testsuite/ChangeLog:

            * gcc.target/i386/pr105066.c: New test.

[Bug target/105066] GCC thinks pinsrw xmm, mem, 0 requires SSE4.1, not SSE2? _mm_loadu_si16 bounces through integer reg

[crazylht at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105066

--- Comment #4 from Hongtao.liu <crazylht at gmail dot com> ---
Fixed in GCC12.

[Bug target/105066] GCC thinks pinsrw xmm, mem, 0 requires SSE4.1, not SSE2? _mm_loadu_si16 bounces through integer reg

[peter at cordes dot ca]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105066

--- Comment #5 from Peter Cordes <peter at cordes dot ca> ---
>     pextrw requires sse4.1 for mem operands.

You're right! I didn't double-check the asm manual for PEXTRW when writing up
the initial report, and had never realized that PINSRW wasn't symmetric with
it.  I was really surprised to see that in
https://www.felixcloutier.com/x86/pextrw

So we do need to care about tuning for _mm_storeu_si16(p, v) without SSE4.1
(without the option of PEXTRW to memory).  PEXTRW to an integer register is
obviously bad; we should be doing

    movd  %xmm0, %eax
    mov   %ax, (%rdi)

instead of an inefficient  pextrw $0, %xmm0, %eax ; movw-store

Reported as PR105079, since the cause of the load missed-opt was GCC thinking
the instruction wasn't available, rather than a wrong tuning choice like this
is.