------- Comment #14 from uweigand at gcc dot gnu dot org  2010-09-03 18:30 
(In reply to comment #12)
> Yes, it would but I think the reload should still generate the right code in
> this particular order of insns.  IMHO, fixing the order of insn is not the
> right thing to do because there might be situation of cycle (e.g. value of 
> p600
> is inherited from 2 but should be reloaded into 3 and p356 is inherited from 3
> and should be reloaded into 2). 
> The problem is definitely in reload inheritance.  Reg_last_reload_reg is not
> invalidated by insn #1407 which is generated by another reload of insn #675.

Actually, I think this really is a reload insn ordering problem. Note that
reload inheritance tracking via reg_last_reload_reg works on the whole reloaded
insn including all the generated reload insns as a whole. Conflicts between
different reloads of the same original insn should be treated specially, taking
into account reload insn ordering (this is done in free_for_value_p).

In this particular case, what happens is this.  After initial reload register
selection, we have this set of reloads:

Reload 0: reload_in (SI) = (reg/v/f:SI 132 [ kpte ])
        reload_in_reg: (reg/v/f:SI 132 [ kpte ])
        reload_reg_rtx: (reg:SI 5 di)
Reload 1: reload_in (SI) = (reg/v/f:SI 132 [ kpte ])
        DIREG, RELOAD_FOR_INPUT (opnum = 1)
        reload_in_reg: (reg/v/f:SI 132 [ kpte ])
        reload_reg_rtx: (reg:SI 5 di)
Reload 2: reload_in (SI) = (reg:SI 588 [ D.29684 ])
        BREG, RELOAD_FOR_INPUT (opnum = 2)
        reload_in_reg: (reg:SI 588 [ D.29684 ])
        reload_reg_rtx: (reg:SI 3 bx)
Reload 3: reload_in (SI) = (reg:SI 356)
        CREG, RELOAD_FOR_INPUT (opnum = 3)
        reload_in_reg: (reg:SI 356)
        reload_reg_rtx: (reg:SI 2 cx)

Reload inheritance notes that reload_in of reload 2 (reg:SI 588) is already
available in CX at this point.  While this cannot be used as reload register
due to the BREG class, it is chosen as "override input", i.e. reload_in is set
to (reg:SI 2 cx) instead of of (reg:SI 588).

Code near the end of choose_reload_regs then verifies whether this causes any
problems due to re-use of the CX register, and comes to the (correct) decision
that it does not, since it knows the reload insn for reload 3 (a
RELOAD_FOR_INPUT for operand 3) which clobbers CX will certainly be generated
*after* the override-input reload insn for reload 2 (a RELOAD_FOR_INPUT for
operand 2).

Unfortunately, some time *after* this decision has been made, the when_needed
field of reload 3 is changed from RELOAD_FOR_INPUT to RELOAD_OTHER.  This
causes the sequence of generated reload insns to change (RELOAD_OTHER reloads
are emitted before RELOAD_FOR_INPUT reloads) -- this breaks the assumptions the
reload inheritance code made, and causes wrong code to be generated.

Now the question is, why does this change to RELOAD_OTHER happen.  This is done
in merge_assigned_reloads, which is called because i386 is a target with
SMALL_REGISTER_CLASSES.  This routine notices that reloads 0 and 1 share a
reload register DI, and decides to merge them, making reload 0 a RELOAD_OTHER
reload (and cancelling reload 1).  It then goes through all other reloads:

          /* If this is now RELOAD_OTHER, look for any reloads that
             load parts of this operand and set them to
             RELOAD_FOR_OTHER_ADDRESS if they were for inputs,
             RELOAD_OTHER for outputs.  Note that this test is
             equivalent to looking for reloads for this operand

This loop now appears to detect reload 3 as a reload that "loads part of" the
operand 0.  This happens because 
               reg_overlap_mentioned_for_reload_p (rld[j].in,
returns true since both reload-in values are stack slots, and
reg_overlap_mentioned_for_reload_p conservatively assumes that all memory
accesses conflict.

Therefore, when_needed for reload 3 is also set to RELOAD_OTHER.  This is not
only unnecessary, but in turn causes the breakage.

Now in this particular case, it might be possible to fix the problem by using a
better detection of which additional reloads need to be modified.  (The comment
says, "Note that this test is equivalent to looking for reloads for this
operand number." -- which is not true, but could be implemented instead ...)

However, the problem seems to be more fundamental.  If *any* change, even
necessary changes, to when_needed flags are made at this point, *any* decision
on reload inheritance or input overrides that was based on reload insn ordering
may now be incorrect.

I guess a conservative fix could be to check whether any reload inheritance was
indeed or input override was indeed performed on this insn, and if so, refuse
to perform the merge ...



Reply via email to