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

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

Richard Sandiford <rsandifo@nildram.co.uk> writes:
> Eric Botcazou <ebotcazou@libertysurf.fr> writes:
>>> But that's the whole idea.  There should be no cases in which register
>>> allocation (viewed as a black box) changes the JUMP_LABEL.
>>
>> What do you mean by 'should'?  'Cannot but I'm not sure', 'Cannot modulo 
>> bugs'?  Because Andrew's change precisely does that and was OKed by RTH.
>
> Cannot modulo bugs.  I've already explained why I think my patch solves
> Andrew's bug too.  It sounds like you think I'm reverting to the situation
> before Andrew's patch, but I'm not really.  Before Andrew's patch, we dealt
> incorrectly with the case where the JUMP_LABEL is not the LABEL_REF we're
> replacing:
>
>   (a) If the JUMP_LABEL was null, we could turn a computed jump into
>       something that neither a computed jump (because of the REG_LABEL
>       note) nor an indirect jump with a known target (because of the
>       null JUMP_LABEL).  This led to an ICE in cfgbuild.
>
>       For avoidance of doubt, this case arose when we had a computed
>       jump whose target didn't get allocated a hard register, but that
>       was known to be equivalent to a LABEL_REF.  We'd replace the
>       target with the LABEL_REF and reload it, and we'd then hit this
>       REG_LABEL code when replacing the LABEL_REF with the reload register.
>
>   (b) If the JUMP_LABEL was some other label, we'd make the JUMP_LABEL
>       inconsistent with the REG_LABEL, thus potentially changing the
>       JUMP_LABEL when rebuild_jump_labels is next called.
>
> So Andrew's patch and mine are dealing with the same case: where the
> LABEL_REF we're replacing is not the JUMP_LABEL.  Andrew's PR was an
> example of (a) and mine is an example of (b).  Andrew's patch fixed
> (a) by making the LABEL_REF the JUMP_LABEL too, thus transforming a
> computed jump into an indirect jump with a known target.  My patch
> fixes (a) by stopping us from adding the note in that case, so that
> the jump in Andrew's PR remains a computed jump.  As I said before,
> transforming a computed jump into an indirect jump to a known target,
> as Andrew did, is a nice CFG optimisation, but not one we should do here.
> It certainly shouldn't be conditional on the register allocators failing
> to find a home for the jump target register (as was the case in Andrew's PR).

BTW, I understand that you want to be conservative here.  It's just that,
from my perspective, keeping the same JUMP_LABEL is more defensive than
changing it.  We're doing no analysis of the instruction pattern here,
so we seem to be changing the JUMP_LABEL on very flimsy evidence.
And, as this PR shows, changing the JUMP_LABEL to an incorrect value
can lead to wrong code.  I think it is much safer to assume that the
pre-reload code has got the JUMP_LABEL right and that reload's job is
simply to preserve this information.  (And from my reading, that's what
the original REG_LABEL code was trying to do.)

The only motivating case we have for changing the JUMP_LABEL is the one
in Andrew's PR, and if we agree that my patch also fixes that case (in
an arguably more conservative way, because it doesn't change the CFG),
then we have no motivating example left.  If your concern is that there
might be examples we haven't considered, either in the discussion of
this patch or of Andrew's, then like I say, I'd argue that keeping the
same JUMP_LABEL is more defensive than changing it.

Richard