Comment 8 for bug 618684

Revision history for this message
Richard Sandiford (rsandifo) wrote :

This is an instance of the bug that Bernd fixed here:

    http://gcc.gnu.org/ml/gcc-patches/2010-03/msg01161.html

We're trying to reload the following ldmsi3 insn:

[insn A]
(insn 28 22 25 2 /home/richards/lp618684.c:18 (parallel [
            (set (reg:SI 0 r0)
                (mem/s:SI (reg/f:SI 25 sfp) [4 nsaddr_list+0 S4 A64]))
            (set (reg:SI 1 r1)
                (mem/s:SI (plus:SI (reg/f:SI 25 sfp)
                        (const_int 4 [0x4])) [4 nsaddr_list+4 S4 A32]))
            (set (reg:SI 2 r2)
                (mem/s:SI (plus:SI (reg/f:SI 25 sfp)
                        (const_int 8 [0x8])) [4 nsaddr_list+8 S4 A64]))
        ]) 189 {*ldmsi3} (nil))

The calculate_needs_all_insns pass tries to determine which reloads
are needed by each instruction. In order to do this, it must have
access to the eliminated form of the instruction, in which things
like the soft frame pointer (sfp) have been replaced by their
"real" sp- or fp-based values. However, the eliminations (and in
particular the elimination offsets) are not final at this stage,
so reload cannot simply replace sfp in-place. It needs to create
a temporary insn:

[insn B]
(insn 28 22 25 2 /home/richards/lp618684.c:18 (parallel [
            (set (reg:SI 0 r0)
                (mem/s:SI (plus:SI (reg/f:SI 13 sp)
                        (const_int 16 [0x10])) [4 nsaddr_list+0 S4 A64]))
            (set (reg:SI 1 r1)
                (mem/s:SI (plus:SI (plus:SI (reg/f:SI 13 sp)
                            (const_int 16 [0x10]))
                        (const_int 4 [0x4])) [4 nsaddr_list+4 S4 A32]))
            (set (reg:SI 2 r2)
                (mem/s:SI (plus:SI (plus:SI (reg/f:SI 13 sp)
                            (const_int 16 [0x10]))
                        (const_int 8 [0x8])) [4 nsaddr_list+8 S4 A64]))
        ]) 189 {*ldmsi3} (nil))

in order to analyse the reloads, then switch back to the original
form before moving to the next instruction.

The task of going from A to B lies with eliminate_regs_in_insn. It:

  1. records the original form of each operand
  2. applies eliminations to each operand
  3. creates a copy (new_body) of the new instruction body
  4. sets the insn's pattern to new_body
  5. restores each operand to its original form, undoing the
     substitutions in 2

Reload can then go from B back to A by setting PATTERN (insn) to
its original value, undoing step 4.

The problem is that, for ldmsi3 instructions, operand 0 comes from
a match_parallel, so is actually the whole instruction body
(PATTERN (insn)). Restoring the original operand 0 in step 5
therefore undoes step 4, and eliminate_regs_in_insn leaves the
insn in its original uneliminated form:

(insn 28 22 25 2 /home/richards/lp618684.c:18 (parallel [
            (set (reg:SI 0 r0)
                (mem/s:SI (reg/f:SI 25 sfp) [4 nsaddr_list+0 S4 A64]))
            (set (reg:SI 1 r1)
                (mem/s:SI (plus:SI (reg/f:SI 25 sfp)
                        (const_int 4 [0x4])) [4 nsaddr_list+4 S4 A32]))
            (set (reg:SI 2 r2)
                (mem/s:SI (plus:SI (reg/f:SI 25 sfp)
                        (const_int 8 [0x8])) [4 nsaddr_list+8 S4 A64]))
        ]) 189 {*ldmsi3} (nil))

And that's the bug: step 5 must skip operands that correspond to
PATTERN (insn). But the failure occurs later. Seeing this insn,
reload thought that no reloads are needed, because the ARM backend
in this release said that the soft frame pointer belongs to
CORE_REGS, and thus to BASE_REG_CLASS, even though the soft frame
pointer is an imaginary GCC-internal construct.

The upshot is that calculate_needs_all_insns thought that this
instruction (A) needs no reloads, then reload_as_needed realised
that the real instruction (B) actually did. That's what the
assert trapped.

Julian's patch to remove the soft frame pointer from CORE_REGS
and GENERAL_REGS (which was for a different bug, and correct IMO)
has fixed this testcase too, because find_reloads can tell from even
the original sfp-based instruction that a reload is needed. However,
find_reload should never have seen the sfp form to begin with, so in
this case, the incorrect output from eliminate_regs_in_insn really is
the underlying bug. It could still trigger in cases where B needs
two reloads (for example because the sp offset is too large).

Bernd's patch is as safe as reload patches get, so I think we should
backport it too as a belt & braces fix.

Richard