[Bug target/114936] New: [14/15 Regression] Typo in aarch64-ldp-fusion.cc:combine_reg_notes

[Bug target/114936] New: [14/15 Regression] Typo in aarch64-ldp-fusion.cc:combine_reg_notes

[acoplan at gcc dot gnu.org]

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

            Bug ID: 114936
           Summary: [14/15 Regression] Typo in
                    aarch64-ldp-fusion.cc:combine_reg_notes
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: acoplan at gcc dot gnu.org
  Target Milestone: ---

aarch64-ldp-fusion.cc:combine_reg_notes has:

  result = filter_notes (REG_NOTES (i2->rtl ()), result,
                         &found_eh_region, fr_expr);
  result = filter_notes (REG_NOTES (i1->rtl ()), result,
                         &found_eh_region, fr_expr + 1);

  if (!load_p)
    {
      // Simple frame-related sp-relative saves don't need CFI notes, but when
      // we combine them into an stp we will need a CFI note as dwarf2cfi can't
      // interpret the unspec pair representation directly.
      if (RTX_FRAME_RELATED_P (i1->rtl ()) && !fr_expr[0])
        fr_expr[0] = copy_rtx (PATTERN (i1->rtl ()));
      if (RTX_FRAME_RELATED_P (i2->rtl ()) && !fr_expr[1])
        fr_expr[1] = copy_rtx (PATTERN (i2->rtl ()));
    }

so any REG_FRAME_RELATED_EXPR from i2 goes to fr_expr[0] and likewise i1 goes
to fr_expr[1], but then we have the opposite association inside the if
statement.

Many thanks to Matthew Malcomson for pointing this out to me.

I'm going to post the (arguably obvious) patch after testing that writes to
fr_expr + 1 first when we call filter_notes for i2.  We may want to consider a
backport to GCC 14 too.

[Bug target/114936] [14/15 Regression] Typo in aarch64-ldp-fusion.cc:combine_reg_notes

[acoplan at gcc dot gnu.org]

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

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-03
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED

[Bug target/114936] [14/15 Regression] Typo in aarch64-ldp-fusion.cc:combine_reg_notes

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |14.2
             Target|                            |aarch64

[Bug target/114936] [14/15 Regression] Typo in aarch64-ldp-fusion.cc:combine_reg_notes

[cvs-commit at gcc dot gnu.org]

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

--- 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:73c8e24b692e691c665d0f1f5424432837bd8c06

commit r15-320-g73c8e24b692e691c665d0f1f5424432837bd8c06
Author: Alex Coplan <alex.coplan@arm.com>
Date:   Fri May 3 14:12:32 2024 +0000

    aarch64: Fix typo in aarch64-ldp-fusion.cc:combine_reg_notes [PR114936]

    This fixes a typo in combine_reg_notes in the load/store pair fusion
    pass.  As it stands, the calls to filter_notes store any
    REG_FRAME_RELATED_EXPR to fr_expr with the following association:

     - i2 -> fr_expr[0]
     - i1 -> fr_expr[1]

    but then the checks inside the following if statement expect the
    opposite (more natural) association, i.e.:

     - i2 -> fr_expr[1]
     - i1 -> fr_expr[0]

    this patch fixes the oversight by swapping the fr_expr indices in the
    calls to filter_notes.

    In hindsight it would probably have been less confusing / error-prone to
    have combine_reg_notes take an array of two insns, then we wouldn't have
    to mix 1-based and 0-based indexing as well as remembering to call
    filter_notes in reverse program order.  This however is a minimal fix
    for backporting purposes.

    gcc/ChangeLog:

            PR target/114936
            * config/aarch64/aarch64-ldp-fusion.cc (combine_reg_notes):
            Ensure insn iN has its REG_FRAME_RELATED_EXPR (if any) stored in
            FR_EXPR[N-1], thus matching the correspondence expected by the
            copy_rtx calls.

[Bug target/114936] [14 Regression] Typo in aarch64-ldp-fusion.cc:combine_reg_notes

[acoplan at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|[14/15 Regression] Typo in  |[14 Regression] Typo in
                   |aarch64-ldp-fusion.cc:combi |aarch64-ldp-fusion.cc:combi
                   |ne_reg_notes                |ne_reg_notes

--- Comment #2 from Alex Coplan <acoplan at gcc dot gnu.org> ---
Fixed on trunk, will backport to 14 after a week or so.