[Bug rtl-optimization/114924] New: [11/12/13/14/15 Regression] Wrong update of MEM_EXPR by RTL loop unrolling since r11-2963-gd6a05b494b4b71

[Bug rtl-optimization/114924] New: [11/12/13/14/15 Regression] Wrong update of MEM_EXPR by RTL loop unrolling since r11-2963-gd6a05b494b4b71

[acoplan at gcc dot gnu.org]

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

            Bug ID: 114924
           Summary: [11/12/13/14/15 Regression] Wrong update of MEM_EXPR
                    by RTL loop unrolling since r11-2963-gd6a05b494b4b71
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: rtl-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: acoplan at gcc dot gnu.org
  Target Milestone: ---

The following testcase is reduced from
libgomp/testsuite/libgomp.fortran/imperfect-destructor.f90:

module m
  type t
    contains
      final fini
  end type
  integer ccount(3)
  contains
    subroutine init(x, n)
      type(t) x
      xi = n
      ccount = 1
    end
    subroutine fini(x)
      type(t) x
      dcount= s1 (a3)
  do i = 1, 1
    block
      do j = 1, 2
        block
          do k = 1, a3
            block
              type (t) local3
              call init (local3, 3)
            end block
          end do
        end block
      end do
    end block
  end do
end
end

compiling with -O2 -funroll-loops -da and looking at the RTL dumps, I see the
following insn in 284r.loop2_invariant:

(insn 44 40 45 8 (set (mem/c:SI (plus:DI (reg/f:DI 121)
                (const_int 8 [0x8])) [3 ccount[2]+0 S4 A64])
        (subreg:SI (reg:V2SI 111) 0)) "t.f90":11:16 discrim 2 69
{*movsi_aarch64}
     (expr_list:REG_DEAD (reg:V2SI 111)
        (nil)))

then in 285r.loop2_unroll, I see:

(insn 44 40 45 8 (set (mem/c:SI (plus:DI (reg/f:DI 121)
                (const_int 8 [0x8])) [3 ccount+0 S4 A64])
        (subreg:SI (reg:V2SI 111) 0)) "t.f90":11:16 discrim 2 69
{*movsi_aarch64}
     (expr_list:REG_DEAD (reg/f:DI 121)
        (expr_list:REG_DEAD (reg:V2SI 111)
            (nil))))

notably the MEM_EXPR has been changed from ccount[2] to ccount, without a
corresponding change in offset.  This is incorrect.  Setting a watchpoint on
the
MEM_ATTRS of the relevant MEM showed that the update happens in
cfgrtl.cc:duplicate_insn_chain, which does the following:

        /* We cannot adjust MR_DEPENDENCE_CLIQUE in-place
           since MEM_EXPR is shared so make a copy and
           walk to the subtree again.  */
        tree new_expr = unshare_expr (MEM_EXPR (*iter));
        if (TREE_CODE (new_expr) == WITH_SIZE_EXPR)
          new_expr = TREE_OPERAND (new_expr, 0);
        while (handled_component_p (new_expr))
          new_expr = TREE_OPERAND (new_expr, 0);
        MR_DEPENDENCE_CLIQUE (new_expr) = newc;
        set_mem_expr (const_cast <rtx> (*iter), new_expr);

so the code (correctly) looks through the ARRAY_REF in this case to find
the underlying MEM_REF and updates MR_DEPENDENCE_CLIQUE for that
MEM_REF, but then proceeds to pass the MEM_REF to set_mem_expr, thereby
incorrectly dropping the ARRAY_REF in this case.

The code above was introduced in
r11-2963-gd6a05b494b4b714e996a5ca09c5a4a1c41dbd648 so I assume this is a
regression in GCC 11 and beyond.

I have a straightforward patch to fix this which passes bootstrap on
aarch64-linux-gnu, I will post that shortly.

While I don't have a wrong-code reproducer at the moment, we may want to
consider backporting the fix as incorrect MEM_EXPR information could
lead to wrong code.  I found the issue while working on a patch series
that has the side effect of introducing some consistency checking of the
MEM_EXPR information.

[Bug rtl-optimization/114924] [11/12/13/14/15 Regression] Wrong update of MEM_EXPR by RTL loop unrolling since r11-2963-gd6a05b494b4b71

[acoplan at gcc dot gnu.org]

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

Alex Coplan <acoplan at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |acoplan at gcc dot gnu.org
   Last reconfirmed|                            |2024-05-02
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1

[Bug rtl-optimization/114924] [11/12/13/14/15 Regression] Wrong update of MEM_EXPR by RTL loop unrolling since r11-2963-gd6a05b494b4b71

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.5
           Keywords|                            |wrong-code
           Priority|P3                          |P2

[Bug rtl-optimization/114924] [11/12/13/14/15 Regression] Wrong update of MEM_EXPR by RTL loop unrolling since r11-2963-gd6a05b494b4b71

[cvs-commit at gcc dot gnu.org]

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

--- Comment #1 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Alex Coplan <acoplan@gcc.gnu.org>:

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

commit r15-126-gfe40d525619eee9c2821126390df75068df4773a
Author: Alex Coplan <alex.coplan@arm.com>
Date:   Fri May 3 09:23:59 2024 +0100

    cfgrtl: Fix MEM_EXPR update in duplicate_insn_chain [PR114924]

    The PR shows that when cfgrtl.cc:duplicate_insn_chain attempts to
    update the MR_DEPENDENCE_CLIQUE information for a MEM_EXPR we can end up
    accidentally dropping (e.g.) an ARRAY_REF from the MEM_EXPR and end up
    replacing it with the underlying MEM_REF.  This leads to an
    inconsistency in the MEM_EXPR information, and could lead to wrong code.

    While the walk down to the MEM_REF is necessary to update
    MR_DEPENDENCE_CLIQUE, we should use the outer tree expression for the
    MEM_EXPR.  This patch does that.

    gcc/ChangeLog:

            PR rtl-optimization/114924
            * cfgrtl.cc (duplicate_insn_chain): When updating MEM_EXPRs,
            don't strip (e.g.) ARRAY_REFs from the final MEM_EXPR.

[Bug rtl-optimization/114924] [11/12/13/14 Regression] Wrong update of MEM_EXPR by RTL loop unrolling since r11-2963-gd6a05b494b4b71

[cvs-commit at gcc dot gnu.org]

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

--- Comment #2 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-14 branch has been updated by Alex Coplan
<acoplan@gcc.gnu.org>:

https://gcc.gnu.org/g:242fbc0df6c23115c47d256e66fba6a770265c5d

commit r14-10160-g242fbc0df6c23115c47d256e66fba6a770265c5d
Author: Alex Coplan <alex.coplan@arm.com>
Date:   Fri May 3 09:23:59 2024 +0100

    cfgrtl: Fix MEM_EXPR update in duplicate_insn_chain [PR114924]

    The PR shows that when cfgrtl.cc:duplicate_insn_chain attempts to
    update the MR_DEPENDENCE_CLIQUE information for a MEM_EXPR we can end up
    accidentally dropping (e.g.) an ARRAY_REF from the MEM_EXPR and end up
    replacing it with the underlying MEM_REF.  This leads to an
    inconsistency in the MEM_EXPR information, and could lead to wrong code.

    While the walk down to the MEM_REF is necessary to update
    MR_DEPENDENCE_CLIQUE, we should use the outer tree expression for the
    MEM_EXPR.  This patch does that.

    gcc/ChangeLog:

            PR rtl-optimization/114924
            * cfgrtl.cc (duplicate_insn_chain): When updating MEM_EXPRs,
            don't strip (e.g.) ARRAY_REFs from the final MEM_EXPR.

    (cherry picked from commit fe40d525619eee9c2821126390df75068df4773a)

[Bug rtl-optimization/114924] [11/12/13 Regression] Wrong update of MEM_EXPR by RTL loop unrolling since r11-2963-gd6a05b494b4b71

[cvs-commit at gcc dot gnu.org]

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

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

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

commit r13-8726-gc63704a2d840436797f54e175a2af0cb029889d2
Author: Alex Coplan <alex.coplan@arm.com>
Date:   Fri May 3 09:23:59 2024 +0100

    cfgrtl: Fix MEM_EXPR update in duplicate_insn_chain [PR114924]

    The PR shows that when cfgrtl.cc:duplicate_insn_chain attempts to
    update the MR_DEPENDENCE_CLIQUE information for a MEM_EXPR we can end up
    accidentally dropping (e.g.) an ARRAY_REF from the MEM_EXPR and end up
    replacing it with the underlying MEM_REF.  This leads to an
    inconsistency in the MEM_EXPR information, and could lead to wrong code.

    While the walk down to the MEM_REF is necessary to update
    MR_DEPENDENCE_CLIQUE, we should use the outer tree expression for the
    MEM_EXPR.  This patch does that.

    gcc/ChangeLog:

            PR rtl-optimization/114924
            * cfgrtl.cc (duplicate_insn_chain): When updating MEM_EXPRs,
            don't strip (e.g.) ARRAY_REFs from the final MEM_EXPR.

    (cherry picked from commit fe40d525619eee9c2821126390df75068df4773a)