[Bug target/80837] New: [7.1.0 regression] x86 accessing a member of a 16-byte atomic object generates terrible code: splitting/merging the bytes

[peter at cordes dot ca]

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

            Bug ID: 80837
           Summary: [7.1.0 regression] x86 accessing a member of a 16-byte
                    atomic object generates terrible code:
                    splitting/merging the bytes
           Product: gcc
           Version: 7.1.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-*-*

This code compiles to much worse asm on gcc7.1.0 than on gcc6 or gcc8, doing an
insane split/merge of separate bytes of the atomic load result.  Sorry I can't
easily check if it's fixed in the latest gcc7, and I don't know what to search
for the check for an existing report.

#include <atomic>
#include <stdint.h>
struct node;
struct alignas(2*sizeof(void*)) counted_ptr {
    node *ptr;    // non-atomic pointer-to-atomic
    uintptr_t count;
    //int foo;
};

node *load_nounion(std::atomic<counted_ptr> *p) {
  return p->load(std::memory_order_acquire).ptr;
}

See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80835 re: using a narrow load
of just the requested member.

See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80833 for the store-forwarding
stalls from a bad int->xmm strategy when compiling this with -m32.

With gcc7.1.0 -mx32 -O3 -march=nehalem (https://godbolt.org/g/jeCwsP)


        movq    (%edi), %rcx   # 8-byte load of the whole object
        xorl    %edx, %edx     # and then split/merge the bytes
        movzbl  %ch, %eax
        movb    %cl, %dl
        movq    %rcx, %rsi
        movb    %al, %dh       # put the 2nd byte back where it belongs, after
getting it into the low byte of eax for no reason.  This could have been movb
%ch, %dh.
        andl    $16711680, %esi
        andl    $4278190080, %ecx
        movzwl  %dx, %eax      # partial-register stall on reading %dx; 3 cycle
stall on core2/nehalem to insert a merging uop (and even compiling with
-march=core2 doesn't change the strategy to use an extra OR insn)
        orq     %rsi, %rax
        orq     %rcx, %rax     # why 64-bit operand-size?
        ret

-m32 uses basically the same code, but with 32-bit OR at the end.  It does the
8-byte atomic load using SSE or x87.  (And then stores/reloads instead of using
movq %xmm0, %eax, but that's a separate missed-optimization)

-m64 (without -msse4) uses a lot of movabs to set up masks for the high bytes
to split/merge all 8.  (The 16B atomic load is done with a call to libatomic
instead of inlining cmpxchg16b since this patch:
https://gcc.gnu.org/ml/gcc-patches/2017-01/msg02344.html).

---

-msse4 results in a different strategy for a useless 8-byte split/merge:

The atomic-load result in rdx:rax is stored to the stack, then reloaded into
xmm1, resulting in a store-forwarding stall.  Looks like the 64-bit version of
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80833: bad int->xmm strategies.


Then the insane byte-at-a-time copy is from the low 8 bytes of %xmm1 into
%xmm0, using pextrb and pinsrb (I'm not going to paste in the whole code), see
https://godbolt.org/g/SCUIeu.

It initializes %xmm0 with
  movq    $0, 16(%rsp)
  movq    $0, 24(%rsp)
  movb    %r10b, 16(%rsp)  # r10 holds the first byte of the source data
  movdqa  16(%rsp), %xmm0
which is obviously horrible compared to movd %r10d, %xmm0.  Or movzbl %al,%eax
/ movd %eax,%xmm0 instead of messing around with %r10 at all. (%r10d is was
written with movzbl (%rsp), %r10d, reloading from the store of RAX.)

IDK how gcc managed to produce code with so many of the small steps done in
horribly sub-optimal ways, nevermind the fact that it's all useless in the
first place.