Re: [4.2 only] RFA: PR 33848: reload rematerialisation of labels

["Ulrich Weigand" <uweigand@de.ibm.com>]

Richard Sandiford wrote:
> Eric Botcazou <ebotcazou@libertysurf.fr> writes:
> > Sorry, I don't seem to be able to make up my mind about this affair or
> > clearly formulate what I'm afraid of.  The thing is, it's reload and
> > I'll never feel comfortable with patching it on a release branch.  So
> > I'd suggest to run this by some other maintainer (RTH?), setting aside
> > the current thread.
> 
> Understood, thanks.  I'll probably wait a few days to see if anyone
> volunteers before I pick a victim.

Richard asked me to have at look at this issue.  Unfortunately, I don't
see any real solution either (except back-porting the full H-P patch).

Looking at the problem from the perspective of finding the most
conservative change, I can see a number of things we should clearly
*not* do:

- Change a non-NULL JUMP_LABEL to something else.
  (Reload must never actually change control flow.)

- Create a jump_insn with REG_LABEL but NULL JUMP_LABEL.
  (This is inconsistent, as Richard points out.)

- More generally, create an insn that a future run of
  rebuild_jump_labels would modify.

But we also should not simply remove an explicit reference to
a label *without* adding a REG_LABEL note, as this could screw
up reference counting on the label and potentially cause it to
be deleted (even though it is still referenced) -- e.g. if the
label has been pushed into the literal pool.


So, ideally, I think we should add the REG_LABEL note and then
recompute the JUMP_LABEL field just like a future run of
rebuild_jump_labels would do.

Now, what value would a future run of rebuild_jump_label create?
Either the label we just added as REG_LABEL, or some other label
(certainly never NULL).  The only reason we might get some other
label is if that label is already mentioned in the insn.  In that
case, we should have already seen a non-NULL JUMP_LABEL prior to 
reload, and reload should not change it.

(However, in the situation where we have a label in the insn that
we are replacing, we should in fact *never* have a non-NULL
JUMP_LABEL -- but apparently we do sometimes ...)


Getting back to the question of what the most conservative fix
for the branch would be, I therefore think it should be something
along the lines of:
- continue to always add the REG_LABEL if we replace a label
- continue to change a NULL JUMP_LABEL to the replaced label
- do *not* change any pre-existing non-NULL JUMP_LABEL

This change should fix the regression in 33848, and it should not
make any other case worse -- it will only change the behaviour of
reload in cases are are obviously broken today ...

Any comments?   Am I overlooking something here?

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  GNU Toolchain for Linux on System z and Cell BE
  Ulrich.Weigand@de.ibm.com