[PATCH][RFC][regrename][sel-sched] Fix arm bootstrap

[Alexander Monakov]

On Thu, 22 Sep 2016, Kyrill Tkachov wrote:
> > I don't follow.  The macro used as a boolean in places changed by your patch
> > is H_F_P_IS_FRAME_POINTER, not H_F_P_REGNUM.
> > 
> > Am I missing something?
> 
> I'm following Bernd's proposed change from:
> https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01330.html

I see - I was misled by the error in the ChangeLog and the fact that the first
hunk is not relevant to the issue:

> diff --git a/gcc/regrename.c b/gcc/regrename.c
> index 54c7768efa226139c340868e42b784fb011a19b9..a7339db441012e338de5697015f04c1fdb970164 100644
> --- a/gcc/regrename.c
> +++ b/gcc/regrename.c
> @@ -464,8 +464,7 @@ rename_chains (void)
>    if (frame_pointer_needed)
>      {
>        add_to_hard_reg_set (&unavailable, Pmode, FRAME_POINTER_REGNUM);
> -      if (!HARD_FRAME_POINTER_IS_FRAME_POINTER)
> -	add_to_hard_reg_set (&unavailable, Pmode, HARD_FRAME_POINTER_REGNUM);
> +      add_to_hard_reg_set (&unavailable, Pmode, HARD_FRAME_POINTER_REGNUM);
>      }

There's no issue here: H_F_P_REGNUM is not used in the boolean context, only
H_F_P_IS_FRAME_POINTER is. There's no warning here, right?

>    FOR_EACH_VEC_ELT (id_to_chain, i, this_head)
> @@ -479,10 +478,9 @@ rename_chains (void)
>  	continue;
>  
>        if (fixed_regs[reg] || global_regs[reg]
> -	  || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
> -	      && reg == HARD_FRAME_POINTER_REGNUM)
> -	  || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
> -	      && reg == FRAME_POINTER_REGNUM))
> +	  || (frame_pointer_needed
> +	      && (reg == HARD_FRAME_POINTER_REGNUM
> +		  || reg == FRAME_POINTER_REGNUM)))
>  	continue;

OK, so here's the real issue: due to a typo 'H_F_P_REGNUM &&
frame_pointer_needed' is used where H_F_P_IS_FRAME_POINTER was used in a
preprocessor #if previously.

A minimal fix would just change H_F_P_REGNUM back to H_F_P_IS_FRAME_POINTER.
I think that's what should be done in order to restore bootstrap: that's clearly
doing no more than restoring previous semantics. The change you've shown also
alters the meaning of the code: I think if that's desired, that should be a
separate patch.

> diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
> index 25a100ee34f6ceaceda2814ae281cadf8b29e688..4a2679c6701c256c5559fa1e9c156bdaad1c2891 100644
> --- a/gcc/sel-sched.c
> +++ b/gcc/sel-sched.c
> @@ -1183,10 +1183,8 @@ mark_unavailable_hard_regs (def_t def, struct reg_rename *reg_rename_p,
>       frame pointer, or we could not discover its class.  */
>    if (fixed_regs[regno]
>        || global_regs[regno]
> -      || (!HARD_FRAME_POINTER_IS_FRAME_POINTER && frame_pointer_needed
> +      || (frame_pointer_needed
>  	  && regno == HARD_FRAME_POINTER_REGNUM)
> -      || (HARD_FRAME_POINTER_REGNUM && frame_pointer_needed
> -	  && regno == FRAME_POINTER_REGNUM)
>        || (reload_completed && cl == NO_REGS))

Originally this condition in sel-sched.c looked exactly like the above in
regrename.c (except the last line).  Please keep them in sync: I think if both
H_F_P_REGNUM and F_P_REGNUM ought to be accepted in rename_chains (like your
patch does), so they should be here in mark_u_h_r.

Thanks.
Alexander