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

[Richard Sandiford <rsandifo@nildram.co.uk>]

Eric Botcazou <ebotcazou@libertysurf.fr> writes:
>> Well, we build jump labels immediately after expanding, and try to keep
>> them up-to-date when changing jumps later.  Not keeping them up-to-date
>> would be a bug.
>
> It's 4.2.x so we have to be conservative and avoid pulling too long a string,
> that's why I would go for a very safe change in this case.
>
> The original code was:
>
> +	  /* If we're replacing a LABEL_REF with a register, add a
> +	     REG_LABEL note to indicate to flow which label this
> +	     register refers to.  */
> +	  if (GET_CODE (*r->where) == LABEL_REF
> +	      && GET_CODE (insn) == JUMP_INSN)
> +	    REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_LABEL,
> +						  XEXP (*r->where, 0),
> +						  REG_NOTES (insn));
>
> Then it was modified as follows
>
> @@ -6033,9 +6033,12 @@ subst_reloads (rtx insn)
>  	     register refers to.  */
>  	  if (GET_CODE (*r->where) == LABEL_REF
>  	      && JUMP_P (insn))
> -	    REG_NOTES (insn) = gen_rtx_INSN_LIST (REG_LABEL,
> -						  XEXP (*r->where, 0),
> -						  REG_NOTES (insn));
> +	    {
> +	      REG_NOTES (insn) = gen_rtx_INSN_LIST (REG_LABEL,
> +						    XEXP (*r->where, 0),
> +						    REG_NOTES (insn));
> +	      JUMP_LABEL (insn) = XEXP (*r->where, 0);
> +	   }
>  
>  	  /* Encapsulate RELOADREG so its machine mode matches what
>  	     used to be there.  Note that gen_lowpart_common will
>
>
> I'm afraid your change would void the original intent in the name of fixing 
> the subsequent modification.  Couldn't we find a middle-ground instead?

I don't think it would reopen the original wound.  The change was for
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20606, and as Andrew says
in comment #3:

------------------------------------------------------------------------
(In reply to comment #2)
> Why is computed_jump_p (insn) not returning true?
Actually it is the following insn:
(jump_insn 169 222 170 19 (set (pc)
        (reg:DI 0 ax)) 515 {*indirect_jump_rtx64} (nil)
    (insn_list:REG_LABEL 172 (nil)))

Which is a computed goto but at this point we have a REG_LABEL which
causes computed_jump_p to return false
------------------------------------------------------------------------

In other words, by unconditionally adding a REG_LABEL note, regardless
of whether there was a JUMP_LABEL, reload was turning something that
satisfied computed_jump_p into something that was neither a computed
jump nor a correct register-indirect jump to a known target.

My patch fixes the same bug by making sure that we only attach
a REG_LABEL note to jumps that already have a known target.
So with my patch, the jump in the testcase would still satsify
computed_jump_p after reload.  And I think it's a valid fix.
While turning a computed jump into a jump to a known target
is an interesting optimisation, this is not the place to do it.
Reload only sees a LABEL_REF here because the original pseudo
did not get allocated a hard register, and the transformation
from a computed jump to a jump to a known target should not
depend on register allocation failure.  The code above is purely
book keeping; the net effect of reload doesn't change the form
of the instruction, which is register-indirect both before and
after register allocation.

Richard