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

[Richard Sandiford <rsandifo at nildram dot co dot uk>]

"Ulrich Weigand" <uweigand@de.ibm.com> writes:
> Richard Sandiford wrote:
>> "Ulrich Weigand" <uweigand@de.ibm.com> writes:
>> > 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.
>> 
>> I don't think this can happen.  The LABEL_N_USES count isn't affected
>> by reload's substitution, so that count should stay accurate until
>> the next call to rebuild_jump_labels.  Before that call, we should
>> never (modulo bugs) reach a situation in which LABEL_N_USES becomes
>> zero and in which this insn still needs it.
>
> Hmm, you're right; the count cannot reach zero when it shouldn't.
> However, the reverse can occur: if delete_insn is called on that
> insn after it was modified by reload, LABEL_N_USES will *not* be
> decremented, even though it should have been, and thus the label
> might not get removed even though it should be ...
>
> This doesn't seem as serious as the reverse problem; but I think
> we've seen actual bugs in the past due to labels that were not
> deleted.

I don't think adding a REG_LABEL note would make any difference as
far as that's concerned either, at least not in the situations we
care about.  Looking at the only deletion routines that handle
REG_LABEL notes:

- cfgrtl.c:delete_insn

  Doesn't consider notes if the jump has a JUMP_LABEL, and we're only
  considering cases where the jump _does_ have a jump label:

  /* If deleting a jump, decrement the use count of the label.  Deleting
     the label itself should happen in the normal course of block merging.  */
  if (JUMP_P (insn)
      && JUMP_LABEL (insn)
      && LABEL_P (JUMP_LABEL (insn)))
    LABEL_NUSES (JUMP_LABEL (insn))--;

  /* Also if deleting an insn that references a label.  */
  else
    {
      while ((note = find_reg_note (insn, REG_LABEL, NULL_RTX)) != NULL_RTX
	     && LABEL_P (XEXP (note, 0)))
	{
	  LABEL_NUSES (XEXP (note, 0))--;
	  remove_note (insn, note);

	}
    }

- flow.c:propagate_block_delete_insn

  Not used for jumps; insn_dead_p doesn't (and shouldn't!) consider sets
  of (pc) to be dead.

- jump.c:delete_related_insns

  The REG_LABEL code isn't executed for jumps:

  /* Likewise for an ordinary INSN / CALL_INSN with a REG_LABEL note.  */
  if (NONJUMP_INSN_P (insn) || CALL_P (insn))
    for (note = REG_NOTES (insn); note; note = XEXP (note, 1))
      if (REG_NOTE_KIND (note) == REG_LABEL
	  /* This could also be a NOTE_INSN_DELETED_LABEL note.  */
	  && LABEL_P (XEXP (note, 0)))
	if (LABEL_NUSES (XEXP (note, 0)) == 0)
	  delete_related_insns (XEXP (note, 0));

> In any case, it just seems more conservative to me to add the note.
> It keeps holding the (existing) LABEL_N_USES count; and it is what
> the code is currently doing.
>
> I'm not sure I've really understood why you're opposed to adding the
> note; would you mind explaining again what your concerns are?

Because it would then have a dual meaning, which is exactly the problem
that H-P was fixing.  H-P's motivating example was a cbranch insn with a
target label operand and a non-target label operand; i.e. one where the
JUMP_LABEL is evident from the jump pattern itself:

    http://gcc.gnu.org/ml/gcc/2005-12/msg00279.html

I'm very reluctant to believe that the 4.2 infrastructure handles this
situation correctly.

As I see it, 4.2 effectively uses label notes on jumps only for encoding
the target, so adding a note for something that isn't the target seems
inherently dangerous.  It's what bit us in this PR.  I'm afraid that
if we keep the note in any other case, we won't actually have a good
theoretical bases for doing it, and the same sort of bug could resurface
in other situations.

Richard