diff mbox

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

Message ID alpine.LNX.2.20.13.1609221721270.14079@monopod.intra.ispras.ru
State New
Headers show

Commit Message

Alexander Monakov Sept. 22, 2016, 2:27 p.m. UTC 
On Thu, 22 Sep 2016, Kyrill Tkachov wrote:
> In the the interest of fixing arm bootstrap here are the two blocking issues
> and the changes proposed for them.
> I'm not familiar enough with regrename or sel-sched to make a call on whether
> these are right or not, I just want to keep the ball rolling so we can fix
> arm bootstrap.
> 
> These changes allowed arm bootstrap to complete.
> Are they the right way to go?
> If so, I'll do a full bootstrap and test run on aarch64 and x86_64.
> 
> Thanks,
> Kyrill
> 
> 2016-09-22  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>             Bernd Edlinger  <bernd.edlinger@hotmail.de>
>             Bernd Schmidt  <bschmidt@redhat.com>
> 
>     * regrename.c (rename_chains): Avoid using HARD_FRAME_POINTER_REGNUM
>     in a boolean context.
>     * sel-sched.c (mark_unavailable_hard_regs): Likewise.

This doesn't look right to me. Note that you're patching uses of
H_F_P_IS_FRAME_POINTER (the ChangeLog is wrong).  As I understand, the issue is
that config/arm/arm.h defines that to plain 0, which causes the warning (ugh?).

Does the following restore bootstrap?

Thanks.
Alexander

Comments

Kyrill Tkachov Sept. 22, 2016, 2:34 p.m. UTC | #1 
On 22/09/16 15:27, Alexander Monakov wrote:
> On Thu, 22 Sep 2016, Kyrill Tkachov wrote:
>> In the the interest of fixing arm bootstrap here are the two blocking issues
>> and the changes proposed for them.
>> I'm not familiar enough with regrename or sel-sched to make a call on whether
>> these are right or not, I just want to keep the ball rolling so we can fix
>> arm bootstrap.
>>
>> These changes allowed arm bootstrap to complete.
>> Are they the right way to go?
>> If so, I'll do a full bootstrap and test run on aarch64 and x86_64.
>>
>> Thanks,
>> Kyrill
>>
>> 2016-09-22  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>              Bernd Edlinger  <bernd.edlinger@hotmail.de>
>>              Bernd Schmidt  <bschmidt@redhat.com>
>>
>>      * regrename.c (rename_chains): Avoid using HARD_FRAME_POINTER_REGNUM
>>      in a boolean context.
>>      * sel-sched.c (mark_unavailable_hard_regs): Likewise.
> This doesn't look right to me. Note that you're patching uses of
> H_F_P_IS_FRAME_POINTER (the ChangeLog is wrong).  As I understand, the issue is
> that config/arm/arm.h defines that to plain 0, which causes the warning (ugh?).
>
> Does the following restore bootstrap?
>
> Thanks.
> Alexander
>
> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
> index 373dc85..1ae82c1 100644
> --- a/gcc/config/arm/arm.h
> +++ b/gcc/config/arm/arm.h
> @@ -887,8 +887,8 @@ extern int arm_arch_crc;
>      ? ARM_HARD_FRAME_POINTER_REGNUM             \
>      : THUMB_HARD_FRAME_POINTER_REGNUM)
>
> -#define HARD_FRAME_POINTER_IS_FRAME_POINTER 0
> -#define HARD_FRAME_POINTER_IS_ARG_POINTER 0
> +#define HARD_FRAME_POINTER_IS_FRAME_POINTER false
> +#define HARD_FRAME_POINTER_IS_ARG_POINTER false

Sorry no, the problem is HARD_FRAME_POINTER_REGNUM that is defined as:
#define HARD_FRAME_POINTER_REGNUM        \
   (TARGET_ARM                    \
    ? ARM_HARD_FRAME_POINTER_REGNUM        \
    : THUMB_HARD_FRAME_POINTER_REGNUM)

where ARM_HARD_FRAME_POINTER_REGNUM is 11 and THUMB_HARD_FRAME_POINTER_REGNUM is 7
so Bernd's new warning triggers whenever HARD_FRAME_POINTER_REGNUM is used as a
boolean like it is in sel-sched.c and regrename.c


>   #define FP_REGNUM                      HARD_FRAME_POINTER_REGNUM
>
>
Alexander Monakov Sept. 22, 2016, 2:39 p.m. UTC | #2 
On Thu, 22 Sep 2016, Kyrill Tkachov wrote:
> Sorry no, the problem is HARD_FRAME_POINTER_REGNUM that is defined as:
> #define HARD_FRAME_POINTER_REGNUM        \
>   (TARGET_ARM                    \
>    ? ARM_HARD_FRAME_POINTER_REGNUM        \
>    : THUMB_HARD_FRAME_POINTER_REGNUM)
> 
> where ARM_HARD_FRAME_POINTER_REGNUM is 11 and THUMB_HARD_FRAME_POINTER_REGNUM
> is 7 so Bernd's new warning triggers whenever HARD_FRAME_POINTER_REGNUM is
> used as a boolean like it is in sel-sched.c and regrename.c

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?

Thanks.
Alexander
Kyrill Tkachov Sept. 22, 2016, 2:45 p.m. UTC | #3 
On 22/09/16 15:39, Alexander Monakov wrote:
> On Thu, 22 Sep 2016, Kyrill Tkachov wrote:
>> Sorry no, the problem is HARD_FRAME_POINTER_REGNUM that is defined as:
>> #define HARD_FRAME_POINTER_REGNUM        \
>>    (TARGET_ARM                    \
>>     ? ARM_HARD_FRAME_POINTER_REGNUM        \
>>     : THUMB_HARD_FRAME_POINTER_REGNUM)
>>
>> where ARM_HARD_FRAME_POINTER_REGNUM is 11 and THUMB_HARD_FRAME_POINTER_REGNUM
>> is 7 so Bernd's new warning triggers whenever HARD_FRAME_POINTER_REGNUM is
>> used as a boolean like it is in sel-sched.c and regrename.c
> 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

that includes removing an extra conditional involving: HARD_FRAME_POINTER_IS_FRAME_POINTER .

I agree the ChangeLog entry should be better. I'm not very familiar with that code, so I
left it generic would something like this be more appropriate?

     * regrename.c (rename_chains): Always add hard frame pointer to unavailable
     set when frame pointer is needed.

Kyrill


> Thanks.
> Alexander
Bernd Edlinger Sept. 22, 2016, 2:54 p.m. UTC | #4 
On 09/22/16 16:34, Kyrill Tkachov wrote:
>
> On 22/09/16 15:27, Alexander Monakov wrote:
>> H_F_P_IS_FRAME_POINTER (the ChangeLog is wrong).  As I understand, the
>> issue is
>> that config/arm/arm.h defines that to plain 0, which causes the
>> warning (ugh?).
>>
>> Does the following restore bootstrap?
>>
>> Thanks.
>> Alexander
>>
>> diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
>> index 373dc85..1ae82c1 100644
>> --- a/gcc/config/arm/arm.h
>> +++ b/gcc/config/arm/arm.h
>> @@ -887,8 +887,8 @@ extern int arm_arch_crc;
>>      ? ARM_HARD_FRAME_POINTER_REGNUM             \
>>      : THUMB_HARD_FRAME_POINTER_REGNUM)
>>
>> -#define HARD_FRAME_POINTER_IS_FRAME_POINTER 0
>> -#define HARD_FRAME_POINTER_IS_ARG_POINTER 0
>> +#define HARD_FRAME_POINTER_IS_FRAME_POINTER false
>> +#define HARD_FRAME_POINTER_IS_ARG_POINTER false
>
> Sorry no, the problem is HARD_FRAME_POINTER_REGNUM that is defined as:
> #define HARD_FRAME_POINTER_REGNUM        \
>    (TARGET_ARM                    \
>     ? ARM_HARD_FRAME_POINTER_REGNUM        \
>     : THUMB_HARD_FRAME_POINTER_REGNUM)
>
> where ARM_HARD_FRAME_POINTER_REGNUM is 11 and
> THUMB_HARD_FRAME_POINTER_REGNUM is 7
> so Bernd's new warning triggers whenever HARD_FRAME_POINTER_REGNUM is
> used as a
> boolean like it is in sel-sched.c and regrename.c
>

Yes and that was obviously a typo.

As of today the warning will no longer trigger if the ?: is in a
macro definition, but most ot the time when the warning triggered
so far, it has pointed to something that needed really our attention
like this one here.

I thinking that there will soon be more different levels of that
warning that may probably not be in -Wall, so we can at least
enable finding code like this.


Bernd.
Alexander Monakov Sept. 22, 2016, 3:24 p.m. UTC | #5 
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
Kyrill Tkachov Sept. 22, 2016, 3:36 p.m. UTC | #6 
On 22/09/16 16:24, Alexander Monakov wrote:
> 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 for the suggestions.
 From what I understand (https://gcc.gnu.org/ml/gcc-patches/2016-09/msg01569.html)
the warning won't trigger anymore for macros, so there shouldn't be a bootstrap
failure any more. I'm re-running an arm bootstrap with latest trunk now to confirm.
If that is the case, then this is not as urgent to fix.

However it's still a problem we should fix now that it has been exposed.
I'll implement your suggestions and do the usual testing.

Thanks,
Kyrill

> Thanks.
> Alexander
diff mbox

Patch

diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
index 373dc85..1ae82c1 100644
--- a/gcc/config/arm/arm.h
+++ b/gcc/config/arm/arm.h
@@ -887,8 +887,8 @@  extern int arm_arch_crc;
    ? ARM_HARD_FRAME_POINTER_REGNUM             \
    : THUMB_HARD_FRAME_POINTER_REGNUM)

-#define HARD_FRAME_POINTER_IS_FRAME_POINTER 0
-#define HARD_FRAME_POINTER_IS_ARG_POINTER 0
+#define HARD_FRAME_POINTER_IS_FRAME_POINTER false
+#define HARD_FRAME_POINTER_IS_ARG_POINTER false

 #define FP_REGNUM                      HARD_FRAME_POINTER_REGNUM