[Bug target/116429] New: [LRA] [M86k] Wrong spill offset

[Bug target/116429] New: [LRA] [M86k] Wrong spill offset

[schwab@linux-m68k.org]

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

            Bug ID: 116429
           Summary: [LRA] [M86k] Wrong spill offset
           Product: gcc
           Version: 15.0
            Status: UNCONFIRMED
          Keywords: ra, wrong-code
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: schwab@linux-m68k.org
            Blocks: 113939
  Target Milestone: ---
            Target: m68k

Created attachment 58961
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=58961&action=edit
io.ii

$ gcc/xg++ -B gcc/ io.ii -std=gnu++26 -S -O2 -mlra io.ii

.L1540:
        move.l %d2,%d0
        and.l 100(%sp),%d0
        cmp.l %d0,%d2
        jne .L1622
        move.l %a0,48(%sp)
        clr.l -(%sp)
        .cfi_def_cfa_offset 96
        pea 37.w
        .cfi_def_cfa_offset 100
        move.l %a1,-(%sp)
        .cfi_def_cfa_offset 104
        move.l %a2,%d0
        sub.l %a1,%d0
        .cfi_def_cfa_offset 108
        move.l %a1,60(%sp)
        move.l %d0,-(%sp)
        jsr (_ZNKSt17basic_string_viewIcSt11char_traitsIcEE4findEcj.isra.0)
        lea (16,%sp),%sp
        .cfi_def_cfa_offset 92
        move.l 44(%sp),%a1
        move.l 48(%sp),%a0

60(%sp) should be 56(%sp), overwriting the value of %a0 at 48(%sp)

From the reload dump:

(insn 1033 549 550 122 (set (mem/c:SI (plus:SI (reg/f:SI 15 %sp)
                (const_int 48 [0x30])) [492 %sfp+-40 S4 A16])
        (reg/v/f:SI 8 %a0 [orig:69 __first ] [69])) 55 {*movsi_m68k2}
     (nil))
(insn 550 1033 551 122 (set (mem:SI (pre_dec:SI (reg/f:SI 15 %sp)) [8  S4 A16])
        (const_int 0 [0]))
"m68k-linux/libstdc++-v3/include/bits/chrono_io.h":478:68 discrim 1 50
{pushexthisi_const}
     (expr_list:REG_ARGS_SIZE (const_int 4 [0x4])
        (nil)))
(insn 551 550 552 122 (set (mem:SI (pre_dec:SI (reg/f:SI 15 %sp)) [8  S4 A16])
        (const_int 37 [0x25]))
"m68k-linux/libstdc++-v3/include/bits/chrono_io.h":478:68 discrim 1 50
{pushexthisi_const}
     (expr_list:REG_ARGS_SIZE (const_int 8 [0x8])
        (nil)))
(insn 552 551 553 122 (set (mem/f:SI (pre_dec:SI (reg/f:SI 15 %sp)) [3  S4
A16])
        (reg/f:SI 9 %a1 [orig:37 _19 ] [37]))
"m68k-linux/libstdc++-v3/include/bits/chrono_io.h":478:68 discrim 1 55
{*movsi_m68k2}
     (expr_list:REG_ARGS_SIZE (const_int 12 [0xc])
        (nil)))
(note 553 552 996 122 NOTE_INSN_DELETED)
(insn 996 553 554 122 (set (reg:SI 0 %d0 [262])
        (reg/v/f:SI 10 %a2 [orig:116 <retval> ] [116]))
"m68k-linux/libstdc++-v3/include/bits/chrono_io.h":478:68 discrim 1 55
{*movsi_m68k2}
     (nil))
(insn 554 996 1031 122 (set (reg:SI 0 %d0 [262])
        (minus:SI (reg:SI 0 %d0 [262])
            (reg/f:SI 9 %a1 [orig:37 _19 ] [37])))
"m68k-linux/libstdc++-v3/include/bits/chrono_io.h":478:68 discrim 1 176
{subsi3}
     (expr_list:REG_ARGS_SIZE (const_int 16 [0x10])
        (nil)))
(insn 1031 554 997 122 (set (mem/c:SI (plus:SI (reg/f:SI 15 %sp)
                (const_int 60 [0x3c])) [492 %sfp+-44 S4 A16])
        (reg/f:SI 9 %a1 [orig:37 _19 ] [37]))
"m68k-linux/libstdc++-v3/include/bits/chrono_io.h":478:68 discrim 1 55
{*movsi_m68k2}
     (nil))
(insn 997 1031 556 122 (set (mem:SI (pre_dec:SI (reg/f:SI 15 %sp)) [8  S4 A16])
        (reg:SI 0 %d0 [262]))
"m68k-linux/libstdc++-v3/include/bits/chrono_io.h":478:68 discrim 1 55
{*movsi_m68k2}
     (nil))
(call_insn/i 556 997 557 122 (set (reg:SI 0 %d0)
        (call (mem:QI (symbol_ref:SI
("_ZNKSt17basic_string_viewIcSt11char_traitsIcEE4findEcj.isra.0") [flags 0x3] 
<function_decl 0x7f3905387a00 find.isra>) [0 find.isra S1 A8])
            (const_int 16 [0x10])))
"m68k-linux/libstdc++-v3/include/bits/chrono_io.h":478:68 discrim 1 393
{*non_symbolic_call_value}
     (expr_list:REG_CALL_DECL (symbol_ref:SI
("_ZNKSt17basic_string_viewIcSt11char_traitsIcEE4findEcj.isra.0") [flags 0x3] 
<function_decl 0x7f3905387a00 find.isra>)
        (expr_list:REG_EH_REGION (const_int 0 [0])
            (nil)))
    (nil))
(insn 557 556 1032 122 (set (reg/f:SI 15 %sp)
        (plus:SI (reg/f:SI 15 %sp)
            (const_int 16 [0x10])))
"m68k-linux/libstdc++-v3/include/bits/chrono_io.h":478:68 discrim 1 150
{*addsi3_internal}
     (expr_list:REG_ARGS_SIZE (const_int 0 [0])
        (nil)))
(insn 1032 557 1034 122 (set (reg/f:SI 9 %a1 [orig:37 _19 ] [37])
        (mem/c:SI (plus:SI (reg/f:SI 15 %sp)
                (const_int 44 [0x2c])) [492 %sfp+-44 S4 A16]))
"m68k-linux/libstdc++-v3/include/bits/chrono_io.h":479:8 55 {*movsi_m68k2}
     (nil))
(insn 1034 1032 559 122 (set (reg/v/f:SI 8 %a0 [orig:69 __first ] [69])
        (mem/c:SI (plus:SI (reg/f:SI 15 %sp)
                (const_int 48 [0x30])) [492 %sfp+-40 S4 A16]))
"m68k-linux/libstdc++-v3/include/bits/chrono_io.h":479:8 55 {*movsi_m68k2}
     (nil))


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113939
[Bug 113939] Switch m68k to LRA

[Bug target/116429] [LRA] [M86k] Wrong spill offset

[matz at gcc dot gnu.org]

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

Michael Matz <matz at gcc dot gnu.org> changed:

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

--- Comment #1 from Michael Matz <matz at gcc dot gnu.org> ---
This is a problem in LRAs setup_sp_offset.  The per-insn sp_offset item is
supposed to reflect the sp_offset before the insn in question.  So, we
(correctly) start with this sequence of insns before LRA, and figure out
(again,
correctly) the sp_offsets noted:

we start at sp_off = 0 
  550: [--sp] = 0             sp_off = 0  {pushexthisi_const}
  551: [--sp] = 37            sp_off = -4 {pushexthisi_const}
  552: [--sp] = r37           sp_off = -8 {movsi_m68k2}
  554: [--sp] = r116 - r37    sp_off = -12 {subsi3}
  556: call                   sp_off = -16

Now, insn 554 (subsi3) doesn't match constraints, the destination is a reg,
needs to match the first input.  So we generate reloads:

      Creating newreg=262, assigning class DATA_REGS to r262
  554: r262:SI=r262:SI-r37:SI
      REG_ARGS_SIZE 0x10
    Inserting insn reload before:
  996: r262:SI=r116:SI
    Inserting insn reload after:
  997: [--%sp:SI]=r262:SI

         Considering alt=0 of insn 997:   (0) =g  (1) damSKT
            1 Non pseudo reload: reject++
          overall=1,losers=0,rld_nregs=0
      Choosing alt 0 in insn 997:  (0) =g  (1) damSKT {*movsi_m68k2}
(sp_off=-16)

Note how insn 997 (the after-reload) now has sp_off=-16 already.  It all
goes downhill from there.  We end up with these insns:

  552: [--sp] = r37           sp_off = -8 {movsi_m68k2}
  996: r262 = r116            sp_off = -12
  554: r262 = r262 - r37      sp_off = -12
  997: [--sp] = r262          sp_off = -16  (!!! should be -12)
  556: call                   sp_off = -16

The call insn sp_off remains at the correct -16, but internally it's already
inconsistent here.  If the sp_off before and insn is -16, and that insn
pre_decs sp, then the after-insn sp_off should be -20.

This is all because setup_sp_offset updates the sp_offset for new insns
incorrectly.  It tries to setup it for the whole (new) insn sequence in order,
but for mysterious reasons starts off with the given sp_offset from the
_next_ instruction after the last insn in the new seq.  That can't ever be
right.  The sp-offset simulation runs in a forward direction (via NEXT_INSN).
The only connection to sp_offset of the next insn after the sequence is that
the offset should end up being equal to that one after simulating the sequence.

(Note how the local varname for the startup point is called 'before' :) )

It's possible that sometime it is correct that the sp_off of the after-last
insn is the correct one to start with.  But then also the simulation of the
insn sequence needs to run backward, not forward.  Generally that probably
can only be decided if setup_sp_offset is given information about on which
of the borders of the sequence it can rely.  But as written it's internally
inconsistent.  Making it consistent as per below fixes this instance of
the problem.  I wouldn't be surprised if it introduces bugs elsewhere,
in which case setup_sp_offset really needs to be amended by more information.

diff --git a/gcc/lra.cc b/gcc/lra.cc
index fb32e134004..5afb21c3ea6 100644
--- a/gcc/lra.cc
+++ b/gcc/lra.cc
@@ -1868,9 +1868,13 @@ push_insns (rtx_insn *from, rtx_insn *to)
 static poly_int64
 setup_sp_offset (rtx_insn *from, rtx_insn *last)
 {
-  rtx_insn *before = next_nonnote_nondebug_insn_bb (last);
-  poly_int64 offset = (before == NULL_RTX || ! INSN_P (before)
-                      ? 0 : lra_get_insn_recog_data (before)->sp_offset);
+  //rtx_insn *before = next_nonnote_nondebug_insn_bb (last);
+  rtx_insn *before = prev_nonnote_nondebug_insn_bb (from);
+  poly_int64 offset = 0;
+
+  if (before && INSN_P (before))
+    offset = lra_update_sp_offset (PATTERN (before),
+                                  lra_get_insn_recog_data
(before)->sp_offset);

   for (rtx_insn *insn = from; insn != NEXT_INSN (last); insn = NEXT_INSN
(insn))
     {

[Bug target/116429] [LRA] [M86k] Wrong spill offset

[cvs-commit at gcc dot gnu.org]

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

--- Comment #2 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Michael Matz <matz@gcc.gnu.org>:

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

commit r15-3221-ge223ac9c225352e3aeea7180a3b56a96ecdbe2fd
Author: Michael Matz <matz@suse.de>
Date:   Thu Aug 22 17:21:42 2024 +0200

    LRA: Fix setup_sp_offset

    This is part of making m68k work with LRA.  See PR116429.
    In short: setup_sp_offset is internally inconsistent.  It wants to
    setup the sp_offset for newly generated instructions.  sp_offset for
    an instruction is always the state of the sp-offset right before that
    instruction.  For that it starts at the (assumed correct) sp_offset
    of the instruction right after the given (new) sequence, and then
    iterates that sequence forward simulating its effects on sp_offset.

    That can't ever be right: either it needs to start at the front
    and simulate forward, or start at the end and simulate backward.
    The former seems to be the more natural way.  Funnily the local
    variable holding that instruction is also called 'before'.

    This changes it to the first variant: start before the sequence,
    do one simulation step to get the sp-offset state in front of the
    sequence and then continue simulating.

    More details: in the problematic testcase we start with this
    situation (sp_off before 550 is 0):

      550: [--sp] = 0             sp_off = 0  {pushexthisi_const}
      551: [--sp] = 37            sp_off = -4 {pushexthisi_const}
      552: [--sp] = r37           sp_off = -8 {movsi_m68k2}
      554: [--sp] = r116 - r37    sp_off = -12 {subsi3}
      556: call                   sp_off = -16

    insn 554 doesn't match its constraints and needs some reloads:

          Creating newreg=262, assigning class DATA_REGS to r262
      554: r262:SI=r262:SI-r37:SI
          REG_ARGS_SIZE 0x10
        Inserting insn reload before:
      996: r262:SI=r116:SI
        Inserting insn reload after:
      997: [--%sp:SI]=r262:SI

             Considering alt=0 of insn 997:   (0) =g  (1) damSKT
                1 Non pseudo reload: reject++
              overall=1,losers=0,rld_nregs=0
          Choosing alt 0 in insn 997:  (0) =g  (1) damSKT {*movsi_m68k2}
(sp_off=-16)

    Note how insn 997 (the after-reload) now has sp_off=-16 already.  It all
    goes downhill from there.  We end up with these insns:

      552: [--sp] = r37           sp_off = -8 {movsi_m68k2}
      996: r262 = r116            sp_off = -12
      554: r262 = r262 - r37      sp_off = -12
      997: [--sp] = r262          sp_off = -16  (!!! should be -12)
      556: call                   sp_off = -16

    The call insn sp_off remains at the correct -16, but internally it's
already
    inconsistent here.  If the sp_off before an insn is -16, and that insn
    pre_decs sp, then the after-insn sp_off should be -20.

            PR target/116429
            * lra.cc (setup_sp_offset): Start with sp_offset from
            before the new sequence, not from after.

[Bug target/116429] [LRA] [M86k] Wrong spill offset

[schwab@linux-m68k.org]

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

Andreas Schwab <schwab@linux-m68k.org> changed:

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

--- Comment #3 from Andreas Schwab <schwab@linux-m68k.org> ---
Fixed for gcc 15.