public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH][RFC][regrename][sel-sched] Fix arm bootstrap
@ 2016-09-22 13:36 Kyrill Tkachov
  2016-09-22 14:34 ` Bernd Edlinger
  2016-09-22 14:37 ` Alexander Monakov
  0 siblings, 2 replies; 9+ messages in thread
From: Kyrill Tkachov @ 2016-09-22 13:36 UTC (permalink / raw)
  To: GCC Patches; +Cc: Bernd Schmidt, Bernd Edlinger

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

Hi all,

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.

[-- Attachment #2: arm-bootstrap.patch --]
[-- Type: text/x-patch, Size: 1852 bytes --]

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);
     }
 
   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;
 
       COPY_HARD_REG_SET (this_unavailable, unavailable);
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))
     {
       SET_HARD_REG_SET (reg_rename_p->unavailable_hard_regs);

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][RFC][regrename][sel-sched] Fix arm bootstrap
  2016-09-22 13:36 [PATCH][RFC][regrename][sel-sched] Fix arm bootstrap Kyrill Tkachov
@ 2016-09-22 14:34 ` Bernd Edlinger
  2016-09-22 14:37 ` Alexander Monakov
  1 sibling, 0 replies; 9+ messages in thread
From: Bernd Edlinger @ 2016-09-22 14:34 UTC (permalink / raw)
  To: Kyrill Tkachov, GCC Patches; +Cc: Bernd Schmidt

On 09/22/16 15:26, Kyrill Tkachov wrote:
> Hi all,
>
> 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.

Thanks Kyill,


--- 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))
      {
        SET_HARD_REG_SET (reg_rename_p->unavailable_hard_regs);

I think, that sel-sched.c would be on the safe side, if it is
staying away from both HARD_FRAME_POINTER and FRAME_POINTER, because
it may apparently also used before reload, where most of the
accesses use FRAME_POINTER.

   if (frame_pointer_needed)
     {
       add_to_hard_reg_set (&reg_rename_p->unavailable_hard_regs,
                            Pmode, FRAME_POINTER_REGNUM);

       if (!HARD_FRAME_POINTER_IS_FRAME_POINTER)
         add_to_hard_reg_set (&reg_rename_p->unavailable_hard_regs,
                              Pmode, HARD_FRAME_POINTER_REGNUM);
     }

also here maybe remove the if (!...) ?



Bernd.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][RFC][regrename][sel-sched] Fix arm bootstrap
  2016-09-22 13:36 [PATCH][RFC][regrename][sel-sched] Fix arm bootstrap Kyrill Tkachov
  2016-09-22 14:34 ` Bernd Edlinger
@ 2016-09-22 14:37 ` Alexander Monakov
  2016-09-22 14:39   ` Kyrill Tkachov
  1 sibling, 1 reply; 9+ messages in thread
From: Alexander Monakov @ 2016-09-22 14:37 UTC (permalink / raw)
  To: Kyrill Tkachov; +Cc: GCC Patches, Bernd Schmidt, Bernd Edlinger

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

 #define FP_REGNUM                      HARD_FRAME_POINTER_REGNUM


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][RFC][regrename][sel-sched] Fix arm bootstrap
  2016-09-22 14:37 ` Alexander Monakov
@ 2016-09-22 14:39   ` Kyrill Tkachov
  2016-09-22 14:52     ` Alexander Monakov
  2016-09-22 15:04     ` Bernd Edlinger
  0 siblings, 2 replies; 9+ messages in thread
From: Kyrill Tkachov @ 2016-09-22 14:39 UTC (permalink / raw)
  To: Alexander Monakov; +Cc: GCC Patches, Bernd Schmidt, Bernd Edlinger


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
>
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][RFC][regrename][sel-sched] Fix arm bootstrap
  2016-09-22 14:39   ` Kyrill Tkachov
@ 2016-09-22 14:52     ` Alexander Monakov
  2016-09-22 14:54       ` Kyrill Tkachov
  2016-09-22 15:04     ` Bernd Edlinger
  1 sibling, 1 reply; 9+ messages in thread
From: Alexander Monakov @ 2016-09-22 14:52 UTC (permalink / raw)
  To: Kyrill Tkachov; +Cc: GCC Patches, Bernd Schmidt, Bernd Edlinger

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][RFC][regrename][sel-sched] Fix arm bootstrap
  2016-09-22 14:52     ` Alexander Monakov
@ 2016-09-22 14:54       ` Kyrill Tkachov
  2016-09-22 15:36         ` Alexander Monakov
  0 siblings, 1 reply; 9+ messages in thread
From: Kyrill Tkachov @ 2016-09-22 14:54 UTC (permalink / raw)
  To: Alexander Monakov; +Cc: GCC Patches, Bernd Schmidt, Bernd Edlinger


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][RFC][regrename][sel-sched] Fix arm bootstrap
  2016-09-22 14:39   ` Kyrill Tkachov
  2016-09-22 14:52     ` Alexander Monakov
@ 2016-09-22 15:04     ` Bernd Edlinger
  1 sibling, 0 replies; 9+ messages in thread
From: Bernd Edlinger @ 2016-09-22 15:04 UTC (permalink / raw)
  To: Kyrill Tkachov, Alexander Monakov; +Cc: GCC Patches, Bernd Schmidt

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][RFC][regrename][sel-sched] Fix arm bootstrap
  2016-09-22 14:54       ` Kyrill Tkachov
@ 2016-09-22 15:36         ` Alexander Monakov
  2016-09-22 15:50           ` Kyrill Tkachov
  0 siblings, 1 reply; 9+ messages in thread
From: Alexander Monakov @ 2016-09-22 15:36 UTC (permalink / raw)
  To: Kyrill Tkachov; +Cc: GCC Patches, Bernd Schmidt, Bernd Edlinger

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH][RFC][regrename][sel-sched] Fix arm bootstrap
  2016-09-22 15:36         ` Alexander Monakov
@ 2016-09-22 15:50           ` Kyrill Tkachov
  0 siblings, 0 replies; 9+ messages in thread
From: Kyrill Tkachov @ 2016-09-22 15:50 UTC (permalink / raw)
  To: Alexander Monakov; +Cc: GCC Patches, Bernd Schmidt, Bernd Edlinger


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-09-22 15:36 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-22 13:36 [PATCH][RFC][regrename][sel-sched] Fix arm bootstrap Kyrill Tkachov
2016-09-22 14:34 ` Bernd Edlinger
2016-09-22 14:37 ` Alexander Monakov
2016-09-22 14:39   ` Kyrill Tkachov
2016-09-22 14:52     ` Alexander Monakov
2016-09-22 14:54       ` Kyrill Tkachov
2016-09-22 15:36         ` Alexander Monakov
2016-09-22 15:50           ` Kyrill Tkachov
2016-09-22 15:04     ` Bernd Edlinger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).