This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

Eric Botcazou <> 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, 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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]