[Bug tree-optimization/108821] New: Extra volatile access with -O2 -ftree-loop-im since GCC-11

[Bug tree-optimization/108821] New: Extra volatile access with -O2 -ftree-loop-im since GCC-11

[sirl at gcc dot gnu.org]

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

            Bug ID: 108821
           Summary: Extra volatile access with -O2 -ftree-loop-im since
                    GCC-11
           Product: gcc
           Version: 11.1.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: sirl at gcc dot gnu.org
  Target Milestone: ---

Hi,

this small example

extern volatile int *x;
static int gCrc;

static int crc16Add(int crc, int b) __attribute__((noinline));
static int crc16Add(int crc, int b)
{
  return crc + b;
}

void f(int data, int dataSz)
{
  int i;

  for(i=0;i<dataSz;i++)
  {
    gCrc = crc16Add(gCrc, data);
    *x = data;
  }
}

adds an extra volatile access after the loop (ARM assembler, but x64 shows the
same problem):

f(int, int):
        mov     r2, r1
        cmp     r2, #0
        ble     .L8
        push    {r3, r4, r5, lr}
        movw    r5, #:lower16:.LANCHOR0
        movt    r5, #:upper16:.LANCHOR0
        movw    r4, #:lower16:x
        movt    r4, #:upper16:x
        mov     r1, r0
        movs    r3, #0
        ldr     r0, [r5]
        ldr     r4, [r4]
.L5:
        adds    r3, r3, #1
        bl      crc16Add(int, int)
        cmp     r2, r3
        str     r1, [r4]  @ <-- the last store here
        bne     .L5
        str     r0, [r5]
        str     r1, [r4]  @ <-- is duplicated here
        pop     {r3, r4, r5, pc}
.L8:
        bx      lr


The tree dumps shows the extra access is added during the lim2 pass.
Compiling with -fno-tree-loop-im avoids the invalid extra access to volatile
memory.

I'm not enough of a language lawyer to be sure that the extra volatile access
is invalid in C/C++, but at least it's a bad optimization.

[Bug tree-optimization/108821] [11/12/13 Regression] LIM reissuing a violatile store when it cannot/should not

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.4
             Status|UNCONFIRMED                 |NEW
           Keywords|                            |wrong-code
   Last reconfirmed|                            |2023-02-16
     Ever confirmed|0                           |1
            Summary|Extra volatile access with  |[11/12/13 Regression] LIM
                   |-O2 -ftree-loop-im since    |reissuing a violatile store
                   |GCC-11                      |when it cannot/should not

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Confirmed.

Re-issueing dependent store of *x.1_3 from loop 1 on exit 3 -> 4
Moving statement
gCrc_lsm.9 = gCrc;
(cost 0) out of loop 1.

[Bug tree-optimization/108821] [11/12/13 Regression] LIM reissuing a violatile store when it cannot/should not

[sirl at gcc dot gnu.org]

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

Franz Sirl <sirl at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenther at suse dot de

--- Comment #2 from Franz Sirl <sirl at gcc dot gnu.org> ---
Started with commit b6ff3ddecfa93d53867afaaa078f85fc848abbbd:

tree-optimization/94988 - enhance SM some more

This enhances store-order preserving store motion to handle the case
of non-invariant dependent stores in the sequence of unconditionally
executed stores on exit by re-issueing them as part of the sequence
of stores on the exit.  This fixes the observed regression of
gcc.target/i386/pr64110.c which relies on store-motion of 'b'
for a loop like

  for (int i = 0; i < j; ++i)
    *b++ = x;

where for correctness we now no longer apply store-motion.  With
the patch we emit the correct

  tem = b;
  for (int i = 0; i < j; ++i)
    {
      tem = tem + 1;
      *tem = x;
    }
  b = tem;
  *tem = x;

preserving the original order of stores.  A testcase reflecting
the miscompilation done by earlier GCC is added as well.

This also fixes the reported ICE in PR95025 and adds checking code
to catch it earlier - the issue was not-supported refs propagation
leaving stray refs in the sequence.

2020-05-11  Richard Biener  <rguenther@suse.de>

PR tree-optimization/94988
PR tree-optimization/95025
* tree-ssa-loop-im.c (seq_entry): Make a struct, add from.
(sm_seq_push_down): Take extra parameter denoting where we
moved the ref to.
(execute_sm_exit): Re-issue sm_other stores in the correct
order.
(sm_seq_valid_bb): When always executed, allow sm_other to
prevail inbetween sm_ord and record their stored value.
(hoist_memory_references): Adjust refs_not_supported propagation
and prune sm_other from the end of the ordered sequences.

* gcc.dg/torture/pr94988.c: New testcase.
* gcc.dg/torture/pr95025.c: Likewise.
* gcc.dg/torture/pr95045.c: Likewise.
* g++.dg/asan/pr95025.C: New testcase.

[Bug tree-optimization/108821] [11/12/13 Regression] LIM reissuing a violatile store when it cannot/should not

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
           Priority|P3                          |P2

--- Comment #3 from Richard Biener <rguenth at gcc dot gnu.org> ---
Mine.

[Bug tree-optimization/108821] [11/12/13 Regression] LIM reissuing a violatile store when it cannot/should not

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:4c4f0f7acd6b96ee744ef598cbea5c7046a33654

commit r13-6114-g4c4f0f7acd6b96ee744ef598cbea5c7046a33654
Author: Richard Biener <rguenther@suse.de>
Date:   Fri Feb 17 12:36:44 2023 +0100

    tree-optimization/108821 - store motion and volatiles

    The following fixes store motion to not re-issue volatile stores
    to preserve TBAA behavior since that will result in the number
    of volatile accesses changing.

            PR tree-optimization/108821
            * tree-ssa-loop-im.cc (sm_seq_valid_bb): We can also not
            move volatile accesses.

            * gcc.dg/tree-ssa/ssa-lim-24.c: New testcase.

[Bug tree-optimization/108821] [11/12 Regression] LIM reissuing a violatile store when it cannot/should not

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[11/12/13 Regression] LIM   |[11/12 Regression] LIM
                   |reissuing a violatile store |reissuing a violatile store
                   |when it cannot/should not   |when it cannot/should not
      Known to work|                            |13.0

--- Comment #5 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed on trunk sofar.

[Bug tree-optimization/108821] [11/12 Regression] LIM reissuing a violatile store when it cannot/should not

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-12 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

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

commit r12-9261-gdbbbaed64f39aae57f5f167174121c1be9d18282
Author: Richard Biener <rguenther@suse.de>
Date:   Fri Feb 17 12:36:44 2023 +0100

    tree-optimization/108821 - store motion and volatiles

    The following fixes store motion to not re-issue volatile stores
    to preserve TBAA behavior since that will result in the number
    of volatile accesses changing.

            PR tree-optimization/108821
            * tree-ssa-loop-im.cc (sm_seq_valid_bb): We can also not
            move volatile accesses.

            * gcc.dg/tree-ssa/ssa-lim-24.c: New testcase.

    (cherry picked from commit 4c4f0f7acd6b96ee744ef598cbea5c7046a33654)

[Bug tree-optimization/108821] [11 Regression] LIM reissuing a violatile store when it cannot/should not

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

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

commit r11-10681-gbd0192968999661d103bb9d974a8ad3be4ef3d9b
Author: Richard Biener <rguenther@suse.de>
Date:   Fri Feb 17 12:36:44 2023 +0100

    tree-optimization/108821 - store motion and volatiles

    The following fixes store motion to not re-issue volatile stores
    to preserve TBAA behavior since that will result in the number
    of volatile accesses changing.

            PR tree-optimization/108821
            * tree-ssa-loop-im.c (sm_seq_valid_bb): We can also not
            move volatile accesses.

            * gcc.dg/tree-ssa/ssa-lim-24.c: New testcase.

    (cherry picked from commit 4c4f0f7acd6b96ee744ef598cbea5c7046a33654)

[Bug tree-optimization/108821] [11 Regression] LIM reissuing a violatile store when it cannot/should not

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |11.3.0
      Known to work|                            |11.3.1
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.  Thanks for reporting.