[Bug c/108498] New: ppc64 big endian generates uninitialized reads with -fstore-merging

[Bug c/108498] New: ppc64 big endian generates uninitialized reads with -fstore-merging

[kungfujesus06 at gmail dot com]

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

            Bug ID: 108498
           Summary: ppc64 big endian generates uninitialized reads with
                    -fstore-merging
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c
          Assignee: unassigned at gcc dot gnu.org
          Reporter: kungfujesus06 at gmail dot com
  Target Milestone: ---

It seems that populating a bit field of many with store merging enabled
produces an access to uninitialized memory.  Here's the minimal reproducer:

```
#include <stdio.h>
#include <stdint.h>

#define NVFX_FP_MASK_X 1
#define NVFX_FP_MASK_Y 2
#define NVFX_FP_MASK_Z 4
#define NVFX_FP_MASK_W 8
#define NVFX_FP_MASK_ALL 0xf
#define NV40_FP_OP_OUT_NONE         (1U << 30)
#define NVFX_FP_OP_OPCODE_MUL 0x02
#define NVFX_COND_TR  7
#define NVFXSR_NONE     0

#define arith(s,o,d,m,s0,s1,s2) \
       nvfx_insn((s), NVFX_FP_OP_OPCODE_##o, -1, \
                       (d), (m), (s0), (s1), (s2))

struct nvfx_reg {
        int8_t type;
        int32_t index;
};

struct nvfx_src {
        struct nvfx_reg reg;

        uint8_t indirect : 1;
        uint8_t indirect_reg : 1;
        uint8_t indirect_swz : 2;
        uint8_t negate : 1;
        uint8_t abs : 1;
        uint8_t swz[4];
};

struct nvfx_insn
{
        uint8_t op;
        char scale;
        int8_t unit;
        uint8_t mask;
        uint8_t cc_swz[4];

        uint8_t sat : 1;
        uint8_t cc_update : 1;
        uint8_t cc_update_reg : 1;
        uint8_t cc_test : 3;
        uint8_t cc_test_reg : 1;

        struct nvfx_reg dst;
        struct nvfx_src src[3];
};

static inline struct nvfx_insn
nvfx_insn(uint8_t sat, unsigned op, int unit, struct nvfx_reg dst, unsigned
mask, struct nvfx_src s0, struct nvfx_src s1, struct nvfx_src s2)
{
        struct nvfx_insn insn = {
                .op = op,
                .scale = 0,
                .unit = unit,
                .sat = sat,
                .mask = mask,
                .cc_update = 0,
                .cc_update_reg = 0,
                .cc_test = NVFX_COND_TR,
                .cc_test_reg = 0,
                .cc_swz = { 0, 1, 2, 3 },
                .dst = dst,
                .src = {s0, s1, s2}
        };
        return insn;
}

static inline struct nvfx_reg
nvfx_reg(int type, int index)
{
        struct nvfx_reg temp = {
                .type = type,
                .index = index,
        };
        return temp;
}

static inline struct nvfx_src
nvfx_src(struct nvfx_reg reg)
{
        struct nvfx_src temp = {
                .reg = reg,
                .abs = 0,
                .negate = 0,
                .swz = { 0, 1, 2, 3 },
                .indirect = 0,
        };
        return temp;
}

struct nvfx_insn emit_test(void)
{
   const struct nvfx_src none = nvfx_src(nvfx_reg(NVFXSR_NONE, 0));
   struct nvfx_insn insn;
   struct nvfx_src src[2];
   struct nvfx_reg tmp = {0, 1};
   int mask, sat, unit = 0;
   int ai = -1, ci = -1, ii = -1;
   int i;

   src[0].reg.type = 0;
   src[0].reg.index = 2;
   src[1].reg.type = 4;
   src[1].reg.index = 8;

   return arith(0, MUL, tmp, NVFX_FP_MASK_X | NVFX_FP_MASK_Y, src[0], src[1],
none);
}

int main(void)
{
    struct nvfx_insn ins = emit_test();
    printf("sat? = %d\n", ins.sat);
}
```

This should print 0, with -fstore-merging it often prints 1.  Valgrind shows
it's access unitialized memory.  The assembly with that optimization disabled
and enabled are in the bug report filed here:

https://gitlab.freedesktop.org/mesa/mesa/-/issues/8134

[Bug c/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[kungfujesus06 at gmail dot com]

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

--- Comment #1 from Adam Stylinski <kungfujesus06 at gmail dot com> ---
I also should note this affects GCC 11.3, as well.  The code being generated
via this optimization, while marginally shorter, seems to be rather suboptimal
in general.  Here's what clang's emitting for the same sequence:

0000000000000930 <.emit_test>:
 930:   3c a0 02 00     lis     r5,512
 934:   38 80 00 01     li      r4,1
 938:   88 e3 00 08     lbz     r7,8(r3)
 93c:   60 a5 ff 03     ori     r5,r5,65283
 940:   38 c0 02 01     li      r6,513
 944:   f8 83 00 0c     std     r4,12(r3)
 948:   38 80 00 02     li      r4,2
 94c:   78 a5 01 8a     rldic   r5,r5,32,6
 950:   f8 83 00 14     std     r4,20(r3)
 954:   38 80 00 00     li      r4,0
 958:   78 c6 d0 02     rotldi  r6,r6,58
 95c:   64 a5 00 01     oris    r5,r5,1
 960:   f8 83 00 34     std     r4,52(r3)
 964:   38 80 00 1c     li      r4,28
 968:   60 a5 02 03     ori     r5,r5,515
 96c:   f8 c3 00 24     std     r6,36(r3)
 970:   54 e6 07 fe     clrlwi  r6,r7,31
 974:   50 c4 07 b4     rlwimi  r4,r6,0,30,26
 978:   f8 a3 00 00     std     r5,0(r3)
 97c:   3c a0 00 01     lis     r5,1
 980:   98 83 00 08     stb     r4,8(r3)
 984:   60 a4 02 03     ori     r4,r5,515
 988:   78 84 c5 c8     rldic   r4,r4,24,23
 98c:   f8 83 00 3c     std     r4,60(r3)
 990:   4e 80 00 20     blr

This bug is affecting the mesa project, so at least fixing this to emitting
correct code (if a little suboptimal) would go a long way.

[Bug middle-end/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|c                           |middle-end

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This might be a valgrind bug of not tracking bits correctly for some ppc
instructions.

[Bug middle-end/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[kungfujesus06 at gmail dot com]

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

--- Comment #3 from Adam Stylinski <kungfujesus06 at gmail dot com> ---
(In reply to Andrew Pinski from comment #2)
> This might be a valgrind bug of not tracking bits correctly for some ppc
> instructions.

It's most certainly not, though.  That bit is explicitly being set to 0 from an
integer and with the optimization enabled it's printing 1.

[Bug middle-end/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[kungfujesus06 at gmail dot com]

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

--- Comment #4 from Adam Stylinski <kungfujesus06 at gmail dot com> ---
Also I strongly suspect valgrind is correctly identifying the unitialized bits
because the underlying bug it produces is a "sometimes" bug, depending on
what's on the heap.  Sometimes insn.sat is 0 (when it behaves correctly),
sometimes it's 1 (black screen).

[Bug middle-end/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Target|                            |powerpc64

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
maybe a stack sharing issue?  Can you try -fstack-reuse=none?

[Bug middle-end/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[kungfujesus06 at gmail dot com]

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

--- Comment #6 from Adam Stylinski <kungfujesus06 at gmail dot com> ---
(In reply to Richard Biener from comment #5)
> maybe a stack sharing issue?  Can you try -fstack-reuse=none?

So that does fix it, at least when the struct is backed by the stack.  And also
valgrind is no longer finding uninitialized memory being used:

adam@g5box ~ $ gcc -O2 -fstack-reuse=none bug_rep2.c -o test.out
adam@g5box ~ $ ./test.out 
sat? = 0

adam@g5box ~ $ valgrind ./test.out
==26982== Memcheck, a memory error detector
==26982== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==26982== Using Valgrind-3.20.0 and LibVEX; rerun with -h for copyright info
==26982== Command: ./test.out
==26982== 
sat? = 0
==26982== 
==26982== HEAP SUMMARY:
==26982==     in use at exit: 0 bytes in 0 blocks
==26982==   total heap usage: 1 allocs, 1 frees, 1,024 bytes allocated
==26982== 
==26982== All heap blocks were freed -- no leaks are possible
==26982== 
==26982== For lists of detected and suppressed errors, rerun with: -s
==26982== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)

Not sure what it might do if backed by the heap, but the code is written in a
way that it initializes the struct on the stack, anyway.

[Bug middle-end/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[kungfujesus06 at gmail dot com]

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

--- Comment #7 from Adam Stylinski <kungfujesus06 at gmail dot com> ---
Err wait, my bad, I had added the workaround in that source code.  The bug
still exists when I take out that pragma to push no store-merging.

adam@g5box ~ $ valgrind ./test.out
==27014== Memcheck, a memory error detector
==27014== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==27014== Using Valgrind-3.20.0 and LibVEX; rerun with -h for copyright info
==27014== Command: ./test.out
==27014== 
==27014== Use of uninitialised value of size 8
==27014==    at 0x415B8C8: _itoa_word (in /lib64/libc.so.6)
==27014==    by 0x4167BE3: __vfprintf_internal (in /lib64/libc.so.6)
==27014==    by 0x100005F7: main (in /home/adam/test.out)
==27014== 
==27014== Conditional jump or move depends on uninitialised value(s)
==27014==    at 0x415B8D0: _itoa_word (in /lib64/libc.so.6)
==27014==    by 0x4167BE3: __vfprintf_internal (in /lib64/libc.so.6)
==27014==    by 0x100005F7: main (in /home/adam/test.out)
==27014== 
==27014== Conditional jump or move depends on uninitialised value(s)
==27014==    at 0x41683F4: __vfprintf_internal (in /lib64/libc.so.6)
==27014==    by 0x4252C13: __printf_chk@@GLIBC_2.4 (in /lib64/libc.so.6)
==27014==    by 0x100005F7: main (in /home/adam/test.out)
==27014== 
==27014== Conditional jump or move depends on uninitialised value(s)
==27014==    at 0x4168FA8: __vfprintf_internal (in /lib64/libc.so.6)
==27014==    by 0x4252C13: __printf_chk@@GLIBC_2.4 (in /lib64/libc.so.6)
==27014==    by 0x100005F7: main (in /home/adam/test.out)
==27014== 
sat? = 0
==27014== 
==27014== HEAP SUMMARY:
==27014==     in use at exit: 0 bytes in 0 blocks
==27014==   total heap usage: 1 allocs, 1 frees, 1,024 bytes allocated
==27014== 
==27014== All heap blocks were freed -- no leaks are possible
==27014== 
==27014== Use --track-origins=yes to see where uninitialised values come from
==27014== For lists of detected and suppressed errors, rerun with: -s
==27014== ERROR SUMMARY: 4 errors from 4 contexts (suppressed: 0 from 0)
adam@g5box ~ $ ./test.out 
sat? = 1

[Bug middle-end/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[kungfujesus06 at gmail dot com]

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

--- Comment #8 from Adam Stylinski <kungfujesus06 at gmail dot com> ---
Here's the code GCC emits:

00000000100007a0 <.emit_test>:
    100007a0:   7c 08 02 a6     mflr    r0
    100007a4:   fb e1 ff f8     std     r31,-8(r1)
    100007a8:   3d 42 ff fe     addis   r10,r2,-2
    100007ac:   81 4a 8a 08     lwz     r10,-30200(r10)
    100007b0:   38 80 00 00     li      r4,0
    100007b4:   39 20 00 00     li      r9,0
    100007b8:   39 60 00 01     li      r11,1
    100007bc:   38 c0 00 02     li      r6,2
    100007c0:   38 e0 00 04     li      r7,4
    100007c4:   79 4a c1 e4     sldi    r10,r10,24
    100007c8:   39 00 00 08     li      r8,8
    100007cc:   38 a0 00 44     li      r5,68
    100007d0:   f8 01 00 10     std     r0,16(r1)
    100007d4:   f8 21 ff 31     stdu    r1,-208(r1)
    100007d8:   3c 00 02 00     lis     r0,512
    100007dc:   60 00 ff 03     ori     r0,r0,65283
    100007e0:   78 00 07 c6     sldi    r0,r0,32
    100007e4:   64 00 00 01     oris    r0,r0,1
    100007e8:   60 00 02 03     ori     r0,r0,515
    100007ec:   eb ed 8f f0     ld      r31,-28688(r13)
    100007f0:   fb e1 00 b8     std     r31,184(r1)
    100007f4:   3b e0 00 00     li      r31,0
    100007f8:   f8 81 00 a8     std     r4,168(r1)
    100007fc:   38 81 00 74     addi    r4,r1,116
    10000800:   f9 41 00 b0     std     r10,176(r1)
    10000804:   99 21 00 80     stb     r9,128(r1)
    10000808:   99 21 00 88     stb     r9,136(r1)
    1000080c:   7c 7f 1b 78     mr      r31,r3
    10000810:   99 21 00 a8     stb     r9,168(r1)
    10000814:   91 21 00 ac     stw     r9,172(r1)
    10000818:   f8 01 00 74     std     r0,116(r1)
    1000081c:   91 61 00 84     stw     r11,132(r1)
    10000820:   90 c1 00 8c     stw     r6,140(r1)
    10000824:   98 e1 00 98     stb     r7,152(r1)
    10000828:   91 01 00 9c     stw     r8,156(r1)
    1000082c:   4b ff fd 15     bl      10000540
<0000003b.plt_call.memcpy@@GLIBC_2.3>
    10000830:   e8 41 00 28     ld      r2,40(r1)
    10000834:   e9 41 00 b8     ld      r10,184(r1)
    10000838:   e9 2d 8f f0     ld      r9,-28688(r13)
    1000083c:   7d 4a 4a 79     xor.    r10,r10,r9
    10000840:   39 20 00 00     li      r9,0
    10000844:   40 82 00 1c     bne     10000860 <.emit_test+0xc0>
    10000848:   38 21 00 d0     addi    r1,r1,208
    1000084c:   7f e3 fb 78     mr      r3,r31
    10000850:   e8 01 00 10     ld      r0,16(r1)
    10000854:   eb e1 ff f8     ld      r31,-8(r1)
    10000858:   7c 08 03 a6     mtlr    r0
    1000085c:   4e 80 00 20     blr
    10000860:   4b ff fd 41     bl      100005a0
<0000003b.plt_call.__stack_chk_fail@@GLIBC_2.4>
    10000864:   e8 41 00 28     ld      r2,40(r1)
    10000868:   00 00 00 00     .long 0x0
    1000086c:   00 00 00 01     .long 0x1
    10000870:   80 01 00 00     lwz     r0,0(r1)
    10000874:   60 00 00 00     nop
    10000878:   00 00 00 00     .long 0x0
    1000087c:   00 01 f7 78     .long 0x1f778

[Bug middle-end/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[pinskia at gcc dot gnu.org]

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

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The only thing is memcpy could be broken ...

I can't find anything wrong with the generated code.


    100007cc:   38 a0 00 44     li      r5,68
...
    100007d8:   3c 00 02 00     lis     r0,512
    100007dc:   60 00 ff 03     ori     r0,r0,65283
    100007e0:   78 00 07 c6     sldi    r0,r0,32
    100007e4:   64 00 00 01     oris    r0,r0,1
    100007e8:   60 00 02 03     ori     r0,r0,515
...
    100007fc:   38 81 00 74     addi    r4,r1,116
...
    1000080c:   7c 7f 1b 78     mr      r31,r3
...
    10000818:   f8 01 00 74     std     r0,116(r1)
...
    1000082c:   4b ff fd 15     bl      10000540
<0000003b.plt_call.memcpy@@GLIBC_2.3>
...





On the main side:
        addi %r3,%r1,116
        ld %r9,-28688(%r13)
        std %r9,184(%r1)
        li %r9,0
        bl emit_test
        lwz %r4,124(%r1)

[Bug middle-end/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[pinskia at gcc dot gnu.org]

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

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
oh wait there is no store to 124 ...

[Bug middle-end/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[kungfujesus06 at gmail dot com]

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

--- Comment #11 from Adam Stylinski <kungfujesus06 at gmail dot com> ---
(In reply to Andrew Pinski from comment #9)
> The only thing is memcpy could be broken ...
> 
> I can't find anything wrong with the generated code.
> 
> 
>     100007cc:	38 a0 00 44 	li      r5,68
> ...
>     100007d8:	3c 00 02 00 	lis     r0,512
>     100007dc:	60 00 ff 03 	ori     r0,r0,65283
>     100007e0:	78 00 07 c6 	sldi    r0,r0,32
>     100007e4:	64 00 00 01 	oris    r0,r0,1
>     100007e8:	60 00 02 03 	ori     r0,r0,515
> ...
>     100007fc:	38 81 00 74 	addi    r4,r1,116
> ...
>     1000080c:	7c 7f 1b 78 	mr      r31,r3
> ...
>     10000818:	f8 01 00 74 	std     r0,116(r1)
> ...
>     1000082c:	4b ff fd 15 	bl      10000540
> <0000003b.plt_call.memcpy@@GLIBC_2.3>
> ...
> 
> 
> 
> 
> 
> On the main side:
>         addi %r3,%r1,116
>         ld %r9,-28688(%r13)
>         std %r9,184(%r1)
>         li %r9,0
>         bl emit_test
>         lwz %r4,124(%r1)

It's possible's a glibc bug and clang avoids it by simply not needing it but it
seems doubtful a small memcpy like this would have an issue that didn't show up
until now.  It seems like if it did need a memcpy, GCC would invoke it's
builtin for a struct like like this rather than call into a library function. 
Is there a compilation flag I can invoke to rule that out?  Like some sort of
"only builtins"?

[Bug middle-end/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[pinskia at gcc dot gnu.org]

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

--- Comment #12 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Adam Stylinski from comment #11) 
> It's possible's a glibc bug and clang avoids it by simply not needing it but
> it seems doubtful a small memcpy like this would have an issue that didn't
> show up until now.  It seems like if it did need a memcpy, GCC would invoke
> it's builtin for a struct like like this rather than call into a library
> function.  Is there a compilation flag I can invoke to rule that out?  Like
> some sort of "only builtins"?

No I misread the generated code. I missed there is a store missing. store to
offset 124 is missing I think.

[Bug tree-optimization/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
          Component|middle-end                  |tree-optimization
   Last reconfirmed|                            |2023-01-23
             Status|UNCONFIRMED                 |NEW

--- Comment #13 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Ok, this seems wrong:

New sequence of 1 stores to replace old one of 10 stores
# .MEM_102 = VDEF <.MEM_101>
MEM <char[8]> [(void *)&insn] = "\x02\x00\xff\x03\x00\x01\x02\x03";
Exceeded original number of stmts (2).  Not profitable to emit new sequence.


The size should be 9 rather 8 ...

[Bug tree-optimization/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[kungfujesus06 at gmail dot com]

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

--- Comment #14 from Adam Stylinski <kungfujesus06 at gmail dot com> ---
(In reply to Andrew Pinski from comment #13)
> Ok, this seems wrong:
> 
> New sequence of 1 stores to replace old one of 10 stores
> # .MEM_102 = VDEF <.MEM_101>
> MEM <char[8]> [(void *)&insn] = "\x02\x00\xff\x03\x00\x01\x02\x03";
> Exceeded original number of stmts (2).  Not profitable to emit new sequence.
> 
> 
> The size should be 9 rather 8 ...

Ah cool.  I guess the suboptimality is probably a bug in its own right.  Any
reason it's using so many stores to memory?  The clang version can accomplish
it almost entirely in GPRs.

[Bug tree-optimization/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[kungfujesus06 at gmail dot com]

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

--- Comment #15 from Adam Stylinski <kungfujesus06 at gmail dot com> ---
(In reply to Adam Stylinski from comment #14)
> (In reply to Andrew Pinski from comment #13)
> > Ok, this seems wrong:
> > 
> > New sequence of 1 stores to replace old one of 10 stores
> > # .MEM_102 = VDEF <.MEM_101>
> > MEM <char[8]> [(void *)&insn] = "\x02\x00\xff\x03\x00\x01\x02\x03";
> > Exceeded original number of stmts (2).  Not profitable to emit new sequence.
> > 
> > 
> > The size should be 9 rather 8 ...
> 
> Ah cool.  I guess the suboptimality is probably a bug in its own right.  Any
> reason it's using so many stores to memory?  The clang version can
> accomplish it almost entirely in GPRs.

I guess "entirely in GPRs" isn't very true.  Clang does it in 7 stores, with
the last being the return value on the stack.  GCC is doing it in 16 stores and
quite a few loads.  The stack churn is a bit unnerving, is there anything that
can be done to improve this?

[Bug tree-optimization/108498] ppc64 big endian generates uninitialized reads with -fstore-merging

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
                 CC|                            |jakub at gcc dot gnu.org
           Assignee|unassigned at gcc dot gnu.org      |jakub at gcc dot gnu.org

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #13)
> Ok, this seems wrong:
> 
> New sequence of 1 stores to replace old one of 10 stores
> # .MEM_102 = VDEF <.MEM_101>
> MEM <char[8]> [(void *)&insn] = "\x02\x00\xff\x03\x00\x01\x02\x03";
> Exceeded original number of stmts (2).  Not profitable to emit new sequence.
> 
> 
> The size should be 9 rather 8 ...

I'll have a look.

[Bug tree-optimization/108498] [11/12/13 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|ppc64 big endian generates  |[11/12/13 Regression] ppc64
                   |uninitialized reads with    |big endian generates
                   |-fstore-merging             |uninitialized reads with
                   |                            |-fstore-merging
           Priority|P3                          |P2
   Target Milestone|---                         |11.4

--- Comment #17 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r11-1810-ge362a897655e3b92949b65a2b53e00fb3ab8ded0

[Bug tree-optimization/108498] [11/12/13 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[jakub at gcc dot gnu.org]

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

--- Comment #18 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Slightly adjusted testcase for -O2 which actually aborts at runtime quite
reliably:

struct A
{
  signed char a1;
  int a2;
};

struct B
{
  struct A b1;
  unsigned char b2:1, b3:1, b4:2, b5:1, b6:1, b7[4];
};

struct C
{
  unsigned char c1;
  char c2;
  signed char c3;
  unsigned char c4, c5[4], c6:1, c7:1, c8:1, c9:3, c10:1;
  struct A c11;
  struct B c12[3];
};

static inline struct C
foo (unsigned char a, unsigned b, int c, struct A d,
     unsigned e, struct B f, struct B g, struct B h)
{
  struct C x
    = { .c1 = b, .c2 = 0, .c3 = c, .c6 = a, .c4 = e, .c7 = 0,
        .c8 = 0, .c9 = 7, .c10 = 0, .c5 = {0, 1, 2, 3}, .c11 = d,
        .c12 = {f, g, h} };
  return x;
}

static inline struct A
bar (int a, int b)
{
  struct A x = { .a1 = a, .a2 = b };
  return x;
}

static inline struct B
baz (struct A b1)
{
  struct B x = { .b1 = b1, .b6 = 0, .b5 = 0, .b7 = {0, 1, 2, 3}, .b2 = 0 };
  return x;
}

struct C
qux (void)
{
  const struct B a = baz (bar (0, 0));
  struct C b;
  struct B c[2];
  struct A d = { 0, 1 };
  c[0].b1.a1 = 0;
  c[0].b1.a2 = 2;
  c[1].b1.a1 = 4;
  c[1].b1.a2 = 8;
  return foo (0, 2, -1, d, 3, c[0], c[1], a);
}

__attribute__((noipa)) void
corge (struct C *x)
{
  char buf[1024];
  __builtin_memset (buf, 0xaa, sizeof (buf));
  asm volatile ("" : : "r" (buf));
  __builtin_memset (x, 0x55, sizeof (struct C));
  asm volatile ("" : : "r" (x));
}

int
main ()
{
  struct C x;
  corge (&x);
  x = qux ();
  if (x.c6 || x.c9 != 7)
    __builtin_abort ();
}

[Bug tree-optimization/108498] [11/12/13 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot gnu.org

--- Comment #19 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
So, the problem is the separate string_concatenate mode introduced in that
r11-1810-ge362a897655e3b92949b65a2b53e00fb3ab8ded0 commit.
This mode is turned on whenever at least one of the stores is using a
STRING_CST and is successfully merged with the other stores.
Now, this mode assumes that the start of the store is on a byte boundary and
that the store ends on a byte boundary too, it simply creates a character array
with try_size / BITS_PER_UNIT and ignores any masking.
It is true that merged_store_group::can_be_merged_into does various checks, if
string_concatenation mode is on, it will reject merging with MEM_REF, etc.,
plus
  /* STRING_CST is compatible with INTEGER_CST if no BIT_INSERT_EXPR.  */
  if (info->rhs_code == STRING_CST
      && stores[0]->rhs_code == INTEGER_CST
      && stores[0]->bitsize == CHAR_BIT)
    return !bit_insertion;

  if (stores[0]->rhs_code == STRING_CST
      && info->rhs_code == INTEGER_CST
      && info->bitsize == CHAR_BIT)
    return !bit_insertion;
and also merged_store_group::do_merge turns this mode off if:
  /* But we cannot use it if we don't have consecutive stores.  */
  if (!consecutive)
    string_concatenation = false;
and on if:
  /* We want to use concatenation if there is any string.  */
  if (info->rhs_code == STRING_CST)
    {
      string_concatenation = true;
      gcc_assert (!bit_insertion);
    }
Now, if the goal is to only do the string_concatenation mode if it is some set
single character stores and at least one or more STRING_CST stores, the above
doesn't achieve that by far.
The first cited hunk above from can_be_merged_into allows it if the first store
is char sized integral store and some later is STRING_CST.  It doesn't check if
that
first store is byte aligned, and doesn't check all the other stores in between.
All do_merge then does is if the stores aren't consecutive the
string_concatenation mode is turned off.  So e.g. we reject merging if there is
2 byte INTEGER_CST store followed by consecutive STRING_CST, but allow 1, 2 and
4 byte INTEGER_CST stores followed by STRING_CST (all consecutive), or say 1
byte, then 3 bits, then STRING_CST etc.
The second one is more strict, when the first store is STRING_CST, we punt on
any following INTEGER_CST store even when it is consecutive, if it isn't 1 byte
store.

Consider e.g. on x86_64-linux with -O2:
struct U { char c[16]; };
struct V { char c[16]; };
struct S { unsigned int a : 3, b : 8, c : 21; struct U d; unsigned int e;
struct V f; unsigned int g : 5, h : 27; };
struct T { unsigned int a : 16, b : 8, c : 8; struct U d; unsigned int e;
struct V f; unsigned int g : 5, h : 27; };

__attribute__((noipa)) void
foo (struct S *p)
{
  p->b = 231;
  p->c = 42;
  p->d = (struct U) { "abcdefghijklmno" };
  p->e = 0xdeadbeef;
  p->f = (struct V) { "ABCDEFGHIJKLMNO" };
}

__attribute__((noipa)) void
bar (struct S *p)
{
  p->b = 231;
  p->c = 42;
  p->d = (struct U) { "abcdefghijklmno" };
  p->e = 0xdeadbeef;
  p->f = (struct V) { "ABCDEFGHIJKLMNO" };
  p->g = 12;
}

__attribute__((noipa)) void
baz (struct T *p)
{
  p->c = 42;
  p->d = (struct U) { "abcdefghijklmno" };
  p->e = 0xdeadbeef;
  p->f = (struct V) { "ABCDEFGHIJKLMNO" };
  p->g = 12;
}

int
main ()
{
  struct S s = {};
  struct T t = {};
  foo (&s);
  if (s.a != 0 || s.b != 231 || s.c != 42
      || __builtin_memcmp (&s.d.c, "abcdefghijklmno", 16) || s.e != 0xdeadbeef
      || __builtin_memcmp (&s.f.c, "ABCDEFGHIJKLMNO", 16) || s.g != 0 || s.h !=
0)
    __builtin_abort ();
  __builtin_memset (&s, 0, sizeof (s));
  s.a = 7;
  s.g = 31;
  s.h = (1U << 27) - 1;
  foo (&s);
  if (s.a != 7 || s.b != 231 || s.c != 42
      || __builtin_memcmp (&s.d.c, "abcdefghijklmno", 16) || s.e != 0xdeadbeef
      || __builtin_memcmp (&s.f.c, "ABCDEFGHIJKLMNO", 16) || s.g != 31 || s.h
!= (1U << 27) - 1)
    __builtin_abort ();
  __builtin_memset (&s, 0, sizeof (s));
  bar (&s);
  if (s.a != 0 || s.b != 231 || s.c != 42
      || __builtin_memcmp (&s.d.c, "abcdefghijklmno", 16) || s.e != 0xdeadbeef
      || __builtin_memcmp (&s.f.c, "ABCDEFGHIJKLMNO", 16) || s.g != 12 || s.h
!= 0)
    __builtin_abort ();
  __builtin_memset (&s, 0, sizeof (s));
  s.a = 7;
  s.g = 31;
  s.h = (1U << 27) - 1;
  bar (&s);
  if (s.a != 7 || s.b != 231 || s.c != 42
      || __builtin_memcmp (&s.d.c, "abcdefghijklmno", 16) || s.e != 0xdeadbeef
      || __builtin_memcmp (&s.f.c, "ABCDEFGHIJKLMNO", 16) || s.g != 12 || s.h
!= (1U << 27) - 1)
    __builtin_abort ();
  baz (&t);
  if (t.a != 0 || t.b != 0 || t.c != 42
      || __builtin_memcmp (&t.d.c, "abcdefghijklmno", 16) || t.e != 0xdeadbeef
      || __builtin_memcmp (&t.f.c, "ABCDEFGHIJKLMNO", 16) || t.g != 12 || t.h
!= 0)
    __builtin_abort ();
  __builtin_memset (&s, 0, sizeof (s));
  t.a = 7;
  t.b = 255;
  t.g = 31;
  t.h = (1U << 27) - 1;
  baz (&t);
  if (t.a != 7 || t.b != 255 || t.c != 42
      || __builtin_memcmp (&t.d.c, "abcdefghijklmno", 16) || t.e != 0xdeadbeef
      || __builtin_memcmp (&t.f.c, "ABCDEFGHIJKLMNO", 16) || t.g != 12 || t.h
!= (1U << 27) - 1)
    __builtin_abort ();
  return 0;
}

The 3 functions show the various issues, non-byte boundary start, non-byte
boundary start & end and non-byte boundary end.

We have lots of options to fix this, the question is what results in best code
generation.  One is to only do what has been probably originally meant, i.e.
only
allow merging of STRING_CSTs with byte aligned byte sized INTEGER_CST stores
(of course consecutive).  Unless we'd want to risk O(n^2) complexity, we might
need a flag that is set in a group with non-byte sized or non-byte aligned
INTEGER_CST stores and if that flag is set, don't allow merging it with a later
STRING_CST.

Another option is keep the current (admittedly weird) behavior but say in
merged_store_group::apply_stores clear string_merging flag if start or width
aren't divisible by BITS_PER_UNIT.  After all, we already clear it also for
buf_size <= MOVE_MAX.

Or some combination of the above, I mean, e.g. in the original testcase as well
as
the https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108498#c18 one the only
STRING_CST in there is small and even power of two sized (and aligned the
same); treating such 4 byte
store differently from INTEGER_CST store in how store merging behaves in that
case is weird.  Of course, another example could be fairly long STRING_CST,
especially with
length not multiple of word size.  We don't really try to estimate that the
STRING_CST
store is actually more expensive than a simple scalar store, so if we decide to
merge
such STRING_CST store with other stores in a group, it could be that we just
punt on it
because the multiple scalar stores look more expensive than the original stores
which have one or more expensive STRING_CST stores.  And if we punt, it might
be the case that
if we didn't merge the STRING_CST store in at first, say the group before the
STRING_CST or after it or both could each store merge successfully and
beneficially.

That said, I think especially for backports to 11/12 and perhaps even for 13
the easiest will be the second option, clear string_merging on non-aligned
start/width.

Eric, your thoughts on this?

[Bug tree-optimization/108498] [11/12/13 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[jakub at gcc dot gnu.org]

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

--- Comment #20 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 54335
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54335&action=edit
gcc13-pr108498.patch

Untested fix (well, checked on both testcases on x86_64-linux -m32/-m64 and
on the first one for powerpc64-linux -m64).

[Bug tree-optimization/108498] [11/12/13 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[ebotcazou at gcc dot gnu.org]

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

--- Comment #21 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> That said, I think especially for backports to 11/12 and perhaps even for 13
> the easiest will be the second option, clear string_merging on non-aligned
> start/width.
> 
> Eric, your thoughts on this?

Yes, nonaligned strings do not make much sense.  Where do they come from in
your first testcase?  Is it fold-const synthesizing STRING_CST from
INTEGER_CST?

[Bug tree-optimization/108498] [11/12/13 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[jakub at gcc dot gnu.org]

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

--- Comment #22 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
The C/C++ FEs since r9-6625-gbec1da64aec26a490 turn some array initializers
into strings.

Anyway, I wonder if for GCC 14 we couldn't just treat STRING_CST the same as
INTEGER_CST during the merging into groups and instead deal with all of that
during split_group.
I.e. only at that time see that for this subset of stores we want to merge them
into one STRING_CST store, these others handle differently, etc.  And the
preconditions for handling something the STRING_CST way would be contiguous
chunk with byte aligned start/end containg at least one longer STRING_CST.  So,
we wouldn't give up e.g. in
the second testcase, but merge the STRING_CSTs stores with the 4 byte
INTEGER_CST store in between them, while handling the bitfield stuff at the
start and/or the end to separate stores.

[Bug tree-optimization/108498] [11/12/13 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[ebotcazou at gcc dot gnu.org]

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

--- Comment #23 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> The C/C++ FEs since r9-6625-gbec1da64aec26a490 turn some array initializers
> into strings.

Ouch.  I'm not sure that's worthwhile if the arrays are not byte-aligned.

> Anyway, I wonder if for GCC 14 we couldn't just treat STRING_CST the same as
> INTEGER_CST during the merging into groups and instead deal with all of that
> during split_group.
> I.e. only at that time see that for this subset of stores we want to merge
> them into one STRING_CST store, these others handle differently, etc.  And
> the preconditions for handling something the STRING_CST way would be
> contiguous chunk with byte aligned start/end containg at least one longer
> STRING_CST.  So, we wouldn't give up e.g. in
> the second testcase, but merge the STRING_CSTs stores with the 4 byte
> INTEGER_CST store in between them, while handling the bitfield stuff at the
> start and/or the end to separate stores.

The handling was separate because STRING_CSTs (+ 1-byte INTEGER_CSTs) were
supposed to be relatively separate from other INTEGER_CSTs, but GCC 9+'s
behavior clearly makes this a bit obsolete.  The implementation is ad-hoc at
best so any idea to streamline it is warmly welcome; for Ada, we are only
interested in bona fide character strings that we want to concatenate as if
they were statically concatenated in the source code.

[Bug tree-optimization/108498] [11/12/13 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[jakub at gcc dot gnu.org]

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

--- Comment #24 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Eric Botcazou from comment #23)
> > The C/C++ FEs since r9-6625-gbec1da64aec26a490 turn some array initializers
> > into strings.
> 
> Ouch.  I'm not sure that's worthwhile if the arrays are not byte-aligned.

They actually are byte aligned.
What happens is basically we have first some INTEGER_CST byte-sized store,
then a STRING_CST store and then an INTEGER_CST store of just a few bits.
So, the conditions for merging the first INTEGER_CST with the STRING_CST are
met,
string_concatenate is set, everything is contiguous and so the bit store at the
end
is merged in (in the testcase there are some other stores in between too, all
INTEGER_CSTs).
In the end we got a group with 71 bits with byte aligned start and we just
store 64 bits and leave those 7 uninitialized.

[Bug tree-optimization/108498] [11/12/13 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[cvs-commit at gcc dot gnu.org]

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

--- Comment #25 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:617be7ba436bcbf9d7b883968c6b3c011206b56c

commit r13-5342-g617be7ba436bcbf9d7b883968c6b3c011206b56c
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Jan 25 10:50:27 2023 +0100

    store-merging: Disable string_concatenate mode if start or end aren't byte
aligned [PR108498]

    The first of the following testcases is miscompiled on powerpc64-linux -O2
    -m64 at least, the latter at least on x86_64-linux -m32/-m64.
    Since GCC 11 store-merging has a separate string_concatenation mode which
    turns stores into setting a MEM_REF from a STRING_CST.
    This mode is triggered if at least one of the to be merged stores
    is a STRING_CST store and either the first store (to earliest address)
    is that STRING_CST store or the first store is 8-bit INTEGER_CST store
    and then there are some rules when to turn that mode off or not merge
    further stores into it.

    The problem with these 2 testcases is that the actual implementation
    relies on start/width of the store to be at byte boundaries, as it
    simply creates a char array, MEM_REF can be only on byte boundaries
    and the char array too, plus obviously STRING_CST as well.
    But as can be easily seen in the second testcase, nothing verifies this,
    while the first store has to be a STRING_CST (which will be aligned)
    or 8-bit INTEGER_CST, that 8-bit INTEGER_CST store could be a bitfield
    store, nothing verifies any stores in between whether they actually are
    8-bit and aligned, the only major requirement is that all the stores
    are consecutive.

    For GCC 14 I think we should reconsider this, simply treat STRING_CST
    stores during the merging like INTEGER_CST stores and deal with it only
    during split_group where we can create multiple parts, this part
    would be a normal store, this part would be STRING_CST store, this part
    another normal store etc.  But that is quite a lot of work, the following
    patch just disables the string_concatenate mode if boundaries aren't byte
    aligned in the spot where we disable it if it is too short too.
    If that happens, we'll just try to do the merging using normal 1/2/4/8 etc.
    byte stores as usually with RMW masking for any bits that shouldn't be
    touched or punt if we end up with too many stores compared to the original.

    Note, an original STRING_CST store will count as one store in that case,
    something we might want to reconsider later too (but, after all,
CONSTRUCTOR
    stores (aka zeroing) already have the same problem, they can be large and
    expensive and we still count them as one store).

    2023-01-25  Jakub Jelinek  <jakub@redhat.com>

            PR tree-optimization/108498
            * gimple-ssa-store-merging.cc (class store_operand_info):
            End coment with full stop rather than comma.
            (split_group): Likewise.
            (merged_store_group::apply_stores): Clear string_concatenation if
            start or end aren't on a byte boundary.

            * gcc.c-torture/execute/pr108498-1.c: New test.
            * gcc.c-torture/execute/pr108498-2.c: New test.

[Bug tree-optimization/108498] [11/12 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[11/12/13 Regression] ppc64 |[11/12 Regression] ppc64
                   |big endian generates        |big endian generates
                   |uninitialized reads with    |uninitialized reads with
                   |-fstore-merging             |-fstore-merging

--- Comment #26 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed on the trunk so far.

[Bug tree-optimization/108498] [11/12 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[cvs-commit at gcc dot gnu.org]

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

--- Comment #27 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:671b7c29dd666cb74dfe5ab01b501d6a0ca7b41c

commit r12-9144-g671b7c29dd666cb74dfe5ab01b501d6a0ca7b41c
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Jan 25 10:50:27 2023 +0100

    store-merging: Disable string_concatenate mode if start or end aren't byte
aligned [PR108498]

    The first of the following testcases is miscompiled on powerpc64-linux -O2
    -m64 at least, the latter at least on x86_64-linux -m32/-m64.
    Since GCC 11 store-merging has a separate string_concatenation mode which
    turns stores into setting a MEM_REF from a STRING_CST.
    This mode is triggered if at least one of the to be merged stores
    is a STRING_CST store and either the first store (to earliest address)
    is that STRING_CST store or the first store is 8-bit INTEGER_CST store
    and then there are some rules when to turn that mode off or not merge
    further stores into it.

    The problem with these 2 testcases is that the actual implementation
    relies on start/width of the store to be at byte boundaries, as it
    simply creates a char array, MEM_REF can be only on byte boundaries
    and the char array too, plus obviously STRING_CST as well.
    But as can be easily seen in the second testcase, nothing verifies this,
    while the first store has to be a STRING_CST (which will be aligned)
    or 8-bit INTEGER_CST, that 8-bit INTEGER_CST store could be a bitfield
    store, nothing verifies any stores in between whether they actually are
    8-bit and aligned, the only major requirement is that all the stores
    are consecutive.

    For GCC 14 I think we should reconsider this, simply treat STRING_CST
    stores during the merging like INTEGER_CST stores and deal with it only
    during split_group where we can create multiple parts, this part
    would be a normal store, this part would be STRING_CST store, this part
    another normal store etc.  But that is quite a lot of work, the following
    patch just disables the string_concatenate mode if boundaries aren't byte
    aligned in the spot where we disable it if it is too short too.
    If that happens, we'll just try to do the merging using normal 1/2/4/8 etc.
    byte stores as usually with RMW masking for any bits that shouldn't be
    touched or punt if we end up with too many stores compared to the original.

    Note, an original STRING_CST store will count as one store in that case,
    something we might want to reconsider later too (but, after all,
CONSTRUCTOR
    stores (aka zeroing) already have the same problem, they can be large and
    expensive and we still count them as one store).

    2023-01-25  Jakub Jelinek  <jakub@redhat.com>

            PR tree-optimization/108498
            * gimple-ssa-store-merging.cc (class store_operand_info):
            End coment with full stop rather than comma.
            (split_group): Likewise.
            (merged_store_group::apply_stores): Clear string_concatenation if
            start or end aren't on a byte boundary.

            * gcc.c-torture/execute/pr108498-1.c: New test.
            * gcc.c-torture/execute/pr108498-2.c: New test.

    (cherry picked from commit 617be7ba436bcbf9d7b883968c6b3c011206b56c)

[Bug tree-optimization/108498] [11 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[11/12 Regression] ppc64    |[11 Regression] ppc64 big
                   |big endian generates        |endian generates
                   |uninitialized reads with    |uninitialized reads with
                   |-fstore-merging             |-fstore-merging

--- Comment #28 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for gcc 12.3 too.

[Bug tree-optimization/108498] [11 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[cvs-commit at gcc dot gnu.org]

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

--- Comment #29 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Jakub Jelinek
<jakub@gcc.gnu.org>:

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

commit r11-10702-gac66ee8147cc1501ce71324aea1c71f9498134af
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Wed Jan 25 10:50:27 2023 +0100

    store-merging: Disable string_concatenate mode if start or end aren't byte
aligned [PR108498]

    The first of the following testcases is miscompiled on powerpc64-linux -O2
    -m64 at least, the latter at least on x86_64-linux -m32/-m64.
    Since GCC 11 store-merging has a separate string_concatenation mode which
    turns stores into setting a MEM_REF from a STRING_CST.
    This mode is triggered if at least one of the to be merged stores
    is a STRING_CST store and either the first store (to earliest address)
    is that STRING_CST store or the first store is 8-bit INTEGER_CST store
    and then there are some rules when to turn that mode off or not merge
    further stores into it.

    The problem with these 2 testcases is that the actual implementation
    relies on start/width of the store to be at byte boundaries, as it
    simply creates a char array, MEM_REF can be only on byte boundaries
    and the char array too, plus obviously STRING_CST as well.
    But as can be easily seen in the second testcase, nothing verifies this,
    while the first store has to be a STRING_CST (which will be aligned)
    or 8-bit INTEGER_CST, that 8-bit INTEGER_CST store could be a bitfield
    store, nothing verifies any stores in between whether they actually are
    8-bit and aligned, the only major requirement is that all the stores
    are consecutive.

    For GCC 14 I think we should reconsider this, simply treat STRING_CST
    stores during the merging like INTEGER_CST stores and deal with it only
    during split_group where we can create multiple parts, this part
    would be a normal store, this part would be STRING_CST store, this part
    another normal store etc.  But that is quite a lot of work, the following
    patch just disables the string_concatenate mode if boundaries aren't byte
    aligned in the spot where we disable it if it is too short too.
    If that happens, we'll just try to do the merging using normal 1/2/4/8 etc.
    byte stores as usually with RMW masking for any bits that shouldn't be
    touched or punt if we end up with too many stores compared to the original.

    Note, an original STRING_CST store will count as one store in that case,
    something we might want to reconsider later too (but, after all,
CONSTRUCTOR
    stores (aka zeroing) already have the same problem, they can be large and
    expensive and we still count them as one store).

    2023-01-25  Jakub Jelinek  <jakub@redhat.com>

            PR tree-optimization/108498
            * gimple-ssa-store-merging.c (class store_operand_info):
            End coment with full stop rather than comma.
            (split_group): Likewise.
            (merged_store_group::apply_stores): Clear string_concatenation if
            start or end aren't on a byte boundary.

            * gcc.c-torture/execute/pr108498-1.c: New test.
            * gcc.c-torture/execute/pr108498-2.c: New test.

    (cherry picked from commit 617be7ba436bcbf9d7b883968c6b3c011206b56c)

[Bug tree-optimization/108498] [11 Regression] ppc64 big endian generates uninitialized reads with -fstore-merging

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #30 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed for 11.4 as well.