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).
OK. Though I fear there may be fallout on this one...
jeff
