On Sun, Aug 25, 2024 at 7:30 AM Jeff Law <jeffreya...@gmail.com> wrote:
>
>
>
> On 8/22/24 9:45 AM, Michael Matz wrote:
> > 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.
> I think you're right in that the current code isn't correct, but the
> natural question is how in the world has this worked to-date. Though I
> guess targets which push arguments are a dying breed (though I would
> have expected i386 to have tripped over this at some point).
Is it because i386 pushes the return address on stack?
> OK. Though I fear there may be fallout on this one...
>
> jeff
>
--
H.J.
