diff mbox series

rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v]

Message ID d5e32c35-254b-b6ef-9637-059c223a765c@linux.ibm.com
State New
Headers
Series rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] |

Commit Message

Kewen.Lin Jan. 13, 2022, 1:55 a.m. UTC 
  Hi,

This patch is to fix register constraint v with
rs6000_constraints[RS6000_CONSTRAINT_v] instead of ALTIVEC_REGS,
just like some other existing register constraints with
RS6000_CONSTRAINT_*.

I happened to see this and hope it's not intentional and just
got neglected. 

Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
powerpc64-linux-gnu P8.

Is it ok for trunk?

BR,
Kewen
-----
gcc/ChangeLog:

	* config/rs6000/constraints.md (register constraint v): Use
	rs6000_constraints[RS6000_CONSTRAINT_v] instead of ALTIVEC_REGS.
---
 gcc/config/rs6000/constraints.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.27.0

Comments

David Edelsohn Jan. 13, 2022, 3:07 a.m. UTC | #1 
On Wed, Jan 12, 2022 at 8:56 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>
> Hi,
>
> This patch is to fix register constraint v with
> rs6000_constraints[RS6000_CONSTRAINT_v] instead of ALTIVEC_REGS,
> just like some other existing register constraints with
> RS6000_CONSTRAINT_*.
>
> I happened to see this and hope it's not intentional and just
> got neglected.
>
> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
> powerpc64-linux-gnu P8.
>
> Is it ok for trunk?

Why do you want to make this change?

rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;

but all of the patterns that use a "v" constraint are (or should be)
protected by TARGET_ALTIVEC, or some final condition that only is
active for TARGET_ALTIVEC.  The other constraints are conditionally
set because they can be used in a pattern with multiple alternatives
where the pattern itself is active but some of the constraints
correspond to NO_REGS when some instruction variants for VSX is not
enabled.

The change isn't wrong, but it doesn't correct a bug and provides no
additional benefit nor clarty that I can see.

Thanks, David
Kewen.Lin Jan. 13, 2022, 3:38 a.m. UTC | #2 
Hi David,

on 2022/1/13 上午11:07, David Edelsohn wrote:
> On Wed, Jan 12, 2022 at 8:56 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>
>> Hi,
>>
>> This patch is to fix register constraint v with
>> rs6000_constraints[RS6000_CONSTRAINT_v] instead of ALTIVEC_REGS,
>> just like some other existing register constraints with
>> RS6000_CONSTRAINT_*.
>>
>> I happened to see this and hope it's not intentional and just
>> got neglected.
>>
>> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
>> powerpc64-linux-gnu P8.
>>
>> Is it ok for trunk?
> 
> Why do you want to make this change?
> 
> rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;
> 
> but all of the patterns that use a "v" constraint are (or should be)
> protected by TARGET_ALTIVEC, or some final condition that only is
> active for TARGET_ALTIVEC.  The other constraints are conditionally
> set because they can be used in a pattern with multiple alternatives
> where the pattern itself is active but some of the constraints
> correspond to NO_REGS when some instruction variants for VSX is not
> enabled.
> 

Good point!  Thanks for the explanation.

> The change isn't wrong, but it doesn't correct a bug and provides no
> additional benefit nor clarty that I can see.
>

The original intention is to make it consistent with the other existing
register constraints with RS6000_CONSTRAINT_*, otherwise it looks a bit
weird (like was neglected).  After you clarified above, RS6000_CONSTRAINT_v
seems useless at all in the current framework.  Do you prefer to remove
it to avoid any confusions instead?

BR,
Kewen

> Thanks, David
>
David Edelsohn Jan. 13, 2022, 3:44 a.m. UTC | #3 
On Wed, Jan 12, 2022 at 10:38 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>
> Hi David,
>
> on 2022/1/13 上午11:07, David Edelsohn wrote:
> > On Wed, Jan 12, 2022 at 8:56 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
> >>
> >> Hi,
> >>
> >> This patch is to fix register constraint v with
> >> rs6000_constraints[RS6000_CONSTRAINT_v] instead of ALTIVEC_REGS,
> >> just like some other existing register constraints with
> >> RS6000_CONSTRAINT_*.
> >>
> >> I happened to see this and hope it's not intentional and just
> >> got neglected.
> >>
> >> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
> >> powerpc64-linux-gnu P8.
> >>
> >> Is it ok for trunk?
> >
> > Why do you want to make this change?
> >
> > rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;
> >
> > but all of the patterns that use a "v" constraint are (or should be)
> > protected by TARGET_ALTIVEC, or some final condition that only is
> > active for TARGET_ALTIVEC.  The other constraints are conditionally
> > set because they can be used in a pattern with multiple alternatives
> > where the pattern itself is active but some of the constraints
> > correspond to NO_REGS when some instruction variants for VSX is not
> > enabled.
> >
>
> Good point!  Thanks for the explanation.
>
> > The change isn't wrong, but it doesn't correct a bug and provides no
> > additional benefit nor clarty that I can see.
> >
>
> The original intention is to make it consistent with the other existing
> register constraints with RS6000_CONSTRAINT_*, otherwise it looks a bit
> weird (like was neglected).  After you clarified above, RS6000_CONSTRAINT_v
> seems useless at all in the current framework.  Do you prefer to remove
> it to avoid any confusions instead?

It's used in the reg_class, so there may be some heuristic in the GCC
register allocator that cares about the number of registers available
for the target.  rs6000_constraints[RS6000_CONSTRAINT_v] is defined
conditionally, so it seems best to leave it as is.

Thanks, David
Kewen.Lin Jan. 13, 2022, 3:56 a.m. UTC | #4 
on 2022/1/13 上午11:44, David Edelsohn wrote:
> On Wed, Jan 12, 2022 at 10:38 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>
>> Hi David,
>>
>> on 2022/1/13 上午11:07, David Edelsohn wrote:
>>> On Wed, Jan 12, 2022 at 8:56 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> This patch is to fix register constraint v with
>>>> rs6000_constraints[RS6000_CONSTRAINT_v] instead of ALTIVEC_REGS,
>>>> just like some other existing register constraints with
>>>> RS6000_CONSTRAINT_*.
>>>>
>>>> I happened to see this and hope it's not intentional and just
>>>> got neglected.
>>>>
>>>> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
>>>> powerpc64-linux-gnu P8.
>>>>
>>>> Is it ok for trunk?
>>>
>>> Why do you want to make this change?
>>>
>>> rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;
>>>
>>> but all of the patterns that use a "v" constraint are (or should be)
>>> protected by TARGET_ALTIVEC, or some final condition that only is
>>> active for TARGET_ALTIVEC.  The other constraints are conditionally
>>> set because they can be used in a pattern with multiple alternatives
>>> where the pattern itself is active but some of the constraints
>>> correspond to NO_REGS when some instruction variants for VSX is not
>>> enabled.
>>>
>>
>> Good point!  Thanks for the explanation.
>>
>>> The change isn't wrong, but it doesn't correct a bug and provides no
>>> additional benefit nor clarty that I can see.
>>>
>>
>> The original intention is to make it consistent with the other existing
>> register constraints with RS6000_CONSTRAINT_*, otherwise it looks a bit
>> weird (like was neglected).  After you clarified above, RS6000_CONSTRAINT_v
>> seems useless at all in the current framework.  Do you prefer to remove
>> it to avoid any confusions instead?
> 
> It's used in the reg_class, so there may be some heuristic in the GCC
> register allocator that cares about the number of registers available
> for the target.  rs6000_constraints[RS6000_CONSTRAINT_v] is defined
> conditionally, so it seems best to leave it as is.
> 

I may miss something, but I didn't find it's used for the above purposes.
If it's best to leave it as is, the proposed patch seems to offer better
readability.

BR,
Kewen
> Thanks, David
>
Kewen.Lin Jan. 13, 2022, 12:28 p.m. UTC | #5 
on 2022/1/13 上午11:56, Kewen.Lin via Gcc-patches wrote:
> on 2022/1/13 上午11:44, David Edelsohn wrote:
>> On Wed, Jan 12, 2022 at 10:38 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>>
>>> Hi David,
>>>
>>> on 2022/1/13 上午11:07, David Edelsohn wrote:
>>>> On Wed, Jan 12, 2022 at 8:56 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> This patch is to fix register constraint v with
>>>>> rs6000_constraints[RS6000_CONSTRAINT_v] instead of ALTIVEC_REGS,
>>>>> just like some other existing register constraints with
>>>>> RS6000_CONSTRAINT_*.
>>>>>
>>>>> I happened to see this and hope it's not intentional and just
>>>>> got neglected.
>>>>>
>>>>> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
>>>>> powerpc64-linux-gnu P8.
>>>>>
>>>>> Is it ok for trunk?
>>>>
>>>> Why do you want to make this change?
>>>>
>>>> rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;
>>>>
>>>> but all of the patterns that use a "v" constraint are (or should be)
>>>> protected by TARGET_ALTIVEC, or some final condition that only is
>>>> active for TARGET_ALTIVEC.  The other constraints are conditionally
>>>> set because they can be used in a pattern with multiple alternatives
>>>> where the pattern itself is active but some of the constraints
>>>> correspond to NO_REGS when some instruction variants for VSX is not
>>>> enabled.
>>>>
>>>
>>> Good point!  Thanks for the explanation.
>>>
>>>> The change isn't wrong, but it doesn't correct a bug and provides no
>>>> additional benefit nor clarty that I can see.
>>>>
>>>
>>> The original intention is to make it consistent with the other existing
>>> register constraints with RS6000_CONSTRAINT_*, otherwise it looks a bit
>>> weird (like was neglected).  After you clarified above, RS6000_CONSTRAINT_v
>>> seems useless at all in the current framework.  Do you prefer to remove
>>> it to avoid any confusions instead?
>>
>> It's used in the reg_class, so there may be some heuristic in the GCC
>> register allocator that cares about the number of registers available
>> for the target.  rs6000_constraints[RS6000_CONSTRAINT_v] is defined
>> conditionally, so it seems best to leave it as is.
>>
> 
> I may miss something, but I didn't find it's used for the above purposes.
> If it's best to leave it as is, the proposed patch seems to offer better
> readability.

Two more inputs for maintainers' decision:

1) the original proposed patch fixed one "bug" that is:

In function rs6000_debug_reg_global, it tries to print the register class
for the register constraint:

  fprintf (stderr,
	   "\n"
	   "d  reg_class = %s\n"
	   "f  reg_class = %s\n"
	   "v  reg_class = %s\n"
	   "wa reg_class = %s\n"
           ...
	   "\n",
	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_d]],
	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_f]],
	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_v]],
	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wa]],
           ...

It uses rs6000_constraints[RS6000_CONSTRAINT_v] which is conditionally
set here:

  /* Add conditional constraints based on various options, to allow us to
     collapse multiple insn patterns.  */
  if (TARGET_ALTIVEC)
    rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;

But the actual register class for register constraint is hardcoded as
ALTIVEC_REGS rather than rs6000_constraints[RS6000_CONSTRAINT_v].

2) Bootstrapped and tested one below patch to remove all the code using
RS6000_CONSTRAINT_v on powerpc64le-linux-gnu P10 and P9,
powerpc64-linux-gnu P8 and P7 with no regressions.

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 37f07fe5358..3652629c5d0 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -2320,7 +2320,6 @@ rs6000_debug_reg_global (void)
 	   "\n"
 	   "d  reg_class = %s\n"
 	   "f  reg_class = %s\n"
-	   "v  reg_class = %s\n"
 	   "wa reg_class = %s\n"
 	   "we reg_class = %s\n"
 	   "wr reg_class = %s\n"
@@ -2329,7 +2328,6 @@ rs6000_debug_reg_global (void)
 	   "\n",
 	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_d]],
 	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_f]],
-	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_v]],
 	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wa]],
 	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_we]],
 	   reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wr]],
@@ -2984,11 +2982,6 @@ rs6000_init_hard_regno_mode_ok (bool global_init_p)
   if (TARGET_VSX)
     rs6000_constraints[RS6000_CONSTRAINT_wa] = VSX_REGS;

-  /* Add conditional constraints based on various options, to allow us to
-     collapse multiple insn patterns.  */
-  if (TARGET_ALTIVEC)
-    rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;
-
   if (TARGET_POWERPC64)
     {
       rs6000_constraints[RS6000_CONSTRAINT_wr] = GENERAL_REGS;
diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
index 4d2f88d4218..48323b80eee 100644
--- a/gcc/config/rs6000/rs6000.h
+++ b/gcc/config/rs6000/rs6000.h
@@ -1237,7 +1237,6 @@ extern enum reg_class rs6000_regno_regclass[FIRST_PSEUDO_REGISTER];
 enum r6000_reg_class_enum {
   RS6000_CONSTRAINT_d,		/* fpr registers for double values */
   RS6000_CONSTRAINT_f,		/* fpr registers for single values */
-  RS6000_CONSTRAINT_v,		/* Altivec registers */
   RS6000_CONSTRAINT_wa,		/* Any VSX register */
   RS6000_CONSTRAINT_we,		/* VSX register if ISA 3.0 vector. */
   RS6000_CONSTRAINT_wr,		/* GPR register if 64-bit  */


BR,
Kewen
David Edelsohn Jan. 13, 2022, 4:31 p.m. UTC | #6 
On Thu, Jan 13, 2022 at 7:28 AM Kewen.Lin <linkw@linux.ibm.com> wrote:
>
> on 2022/1/13 上午11:56, Kewen.Lin via Gcc-patches wrote:
> > on 2022/1/13 上午11:44, David Edelsohn wrote:
> >> On Wed, Jan 12, 2022 at 10:38 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
> >>>
> >>> Hi David,
> >>>
> >>> on 2022/1/13 上午11:07, David Edelsohn wrote:
> >>>> On Wed, Jan 12, 2022 at 8:56 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
> >>>>>
> >>>>> Hi,
> >>>>>
> >>>>> This patch is to fix register constraint v with
> >>>>> rs6000_constraints[RS6000_CONSTRAINT_v] instead of ALTIVEC_REGS,
> >>>>> just like some other existing register constraints with
> >>>>> RS6000_CONSTRAINT_*.
> >>>>>
> >>>>> I happened to see this and hope it's not intentional and just
> >>>>> got neglected.
> >>>>>
> >>>>> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
> >>>>> powerpc64-linux-gnu P8.
> >>>>>
> >>>>> Is it ok for trunk?
> >>>>
> >>>> Why do you want to make this change?
> >>>>
> >>>> rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;
> >>>>
> >>>> but all of the patterns that use a "v" constraint are (or should be)
> >>>> protected by TARGET_ALTIVEC, or some final condition that only is
> >>>> active for TARGET_ALTIVEC.  The other constraints are conditionally
> >>>> set because they can be used in a pattern with multiple alternatives
> >>>> where the pattern itself is active but some of the constraints
> >>>> correspond to NO_REGS when some instruction variants for VSX is not
> >>>> enabled.
> >>>>
> >>>
> >>> Good point!  Thanks for the explanation.
> >>>
> >>>> The change isn't wrong, but it doesn't correct a bug and provides no
> >>>> additional benefit nor clarty that I can see.
> >>>>
> >>>
> >>> The original intention is to make it consistent with the other existing
> >>> register constraints with RS6000_CONSTRAINT_*, otherwise it looks a bit
> >>> weird (like was neglected).  After you clarified above, RS6000_CONSTRAINT_v
> >>> seems useless at all in the current framework.  Do you prefer to remove
> >>> it to avoid any confusions instead?
> >>
> >> It's used in the reg_class, so there may be some heuristic in the GCC
> >> register allocator that cares about the number of registers available
> >> for the target.  rs6000_constraints[RS6000_CONSTRAINT_v] is defined
> >> conditionally, so it seems best to leave it as is.
> >>
> >
> > I may miss something, but I didn't find it's used for the above purposes.
> > If it's best to leave it as is, the proposed patch seems to offer better
> > readability.
>
> Two more inputs for maintainers' decision:
>
> 1) the original proposed patch fixed one "bug" that is:
>
> In function rs6000_debug_reg_global, it tries to print the register class
> for the register constraint:
>
>   fprintf (stderr,
>            "\n"
>            "d  reg_class = %s\n"
>            "f  reg_class = %s\n"
>            "v  reg_class = %s\n"
>            "wa reg_class = %s\n"
>            ...
>            "\n",
>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_d]],
>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_f]],
>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_v]],
>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wa]],
>            ...
>
> It uses rs6000_constraints[RS6000_CONSTRAINT_v] which is conditionally
> set here:
>
>   /* Add conditional constraints based on various options, to allow us to
>      collapse multiple insn patterns.  */
>   if (TARGET_ALTIVEC)
>     rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;
>
> But the actual register class for register constraint is hardcoded as
> ALTIVEC_REGS rather than rs6000_constraints[RS6000_CONSTRAINT_v].

I agree that the information is inaccurate, but it is informal
debugging output.  And if Altivec is disabled, the value of the
constraint is irrelevant / garbage.

>
> 2) Bootstrapped and tested one below patch to remove all the code using
> RS6000_CONSTRAINT_v on powerpc64le-linux-gnu P10 and P9,
> powerpc64-linux-gnu P8 and P7 with no regressions.
>
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index 37f07fe5358..3652629c5d0 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -2320,7 +2320,6 @@ rs6000_debug_reg_global (void)
>            "\n"
>            "d  reg_class = %s\n"
>            "f  reg_class = %s\n"
> -          "v  reg_class = %s\n"
>            "wa reg_class = %s\n"
>            "we reg_class = %s\n"
>            "wr reg_class = %s\n"
> @@ -2329,7 +2328,6 @@ rs6000_debug_reg_global (void)
>            "\n",
>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_d]],
>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_f]],
> -          reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_v]],
>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wa]],
>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_we]],
>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wr]],
> @@ -2984,11 +2982,6 @@ rs6000_init_hard_regno_mode_ok (bool global_init_p)
>    if (TARGET_VSX)
>      rs6000_constraints[RS6000_CONSTRAINT_wa] = VSX_REGS;
>
> -  /* Add conditional constraints based on various options, to allow us to
> -     collapse multiple insn patterns.  */
> -  if (TARGET_ALTIVEC)
> -    rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;
> -
>    if (TARGET_POWERPC64)
>      {
>        rs6000_constraints[RS6000_CONSTRAINT_wr] = GENERAL_REGS;
> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
> index 4d2f88d4218..48323b80eee 100644
> --- a/gcc/config/rs6000/rs6000.h
> +++ b/gcc/config/rs6000/rs6000.h
> @@ -1237,7 +1237,6 @@ extern enum reg_class rs6000_regno_regclass[FIRST_PSEUDO_REGISTER];
>  enum r6000_reg_class_enum {
>    RS6000_CONSTRAINT_d,         /* fpr registers for double values */
>    RS6000_CONSTRAINT_f,         /* fpr registers for single values */
> -  RS6000_CONSTRAINT_v,         /* Altivec registers */
>    RS6000_CONSTRAINT_wa,                /* Any VSX register */
>    RS6000_CONSTRAINT_we,                /* VSX register if ISA 3.0 vector. */
>    RS6000_CONSTRAINT_wr,                /* GPR register if 64-bit  */

I would prefer that we not make gratuitous changes to this code, but
maybe Segher has a different opinion.

Thanks, David
Kewen.Lin Jan. 26, 2022, 2:26 a.m. UTC | #7 
on 2022/1/14 上午12:31, David Edelsohn wrote:
> On Thu, Jan 13, 2022 at 7:28 AM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>
>> on 2022/1/13 上午11:56, Kewen.Lin via Gcc-patches wrote:
>>> on 2022/1/13 上午11:44, David Edelsohn wrote:
>>>> On Wed, Jan 12, 2022 at 10:38 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>>>>
>>>>> Hi David,
>>>>>
>>>>> on 2022/1/13 上午11:07, David Edelsohn wrote:
>>>>>> On Wed, Jan 12, 2022 at 8:56 PM Kewen.Lin <linkw@linux.ibm.com> wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> This patch is to fix register constraint v with
>>>>>>> rs6000_constraints[RS6000_CONSTRAINT_v] instead of ALTIVEC_REGS,
>>>>>>> just like some other existing register constraints with
>>>>>>> RS6000_CONSTRAINT_*.
>>>>>>>
>>>>>>> I happened to see this and hope it's not intentional and just
>>>>>>> got neglected.
>>>>>>>
>>>>>>> Bootstrapped and regtested on powerpc64le-linux-gnu P9 and
>>>>>>> powerpc64-linux-gnu P8.
>>>>>>>
>>>>>>> Is it ok for trunk?
>>>>>>
>>>>>> Why do you want to make this change?
>>>>>>
>>>>>> rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;
>>>>>>
>>>>>> but all of the patterns that use a "v" constraint are (or should be)
>>>>>> protected by TARGET_ALTIVEC, or some final condition that only is
>>>>>> active for TARGET_ALTIVEC.  The other constraints are conditionally
>>>>>> set because they can be used in a pattern with multiple alternatives
>>>>>> where the pattern itself is active but some of the constraints
>>>>>> correspond to NO_REGS when some instruction variants for VSX is not
>>>>>> enabled.
>>>>>>
>>>>>
>>>>> Good point!  Thanks for the explanation.
>>>>>
>>>>>> The change isn't wrong, but it doesn't correct a bug and provides no
>>>>>> additional benefit nor clarty that I can see.
>>>>>>
>>>>>
>>>>> The original intention is to make it consistent with the other existing
>>>>> register constraints with RS6000_CONSTRAINT_*, otherwise it looks a bit
>>>>> weird (like was neglected).  After you clarified above, RS6000_CONSTRAINT_v
>>>>> seems useless at all in the current framework.  Do you prefer to remove
>>>>> it to avoid any confusions instead?
>>>>
>>>> It's used in the reg_class, so there may be some heuristic in the GCC
>>>> register allocator that cares about the number of registers available
>>>> for the target.  rs6000_constraints[RS6000_CONSTRAINT_v] is defined
>>>> conditionally, so it seems best to leave it as is.
>>>>
>>>
>>> I may miss something, but I didn't find it's used for the above purposes.
>>> If it's best to leave it as is, the proposed patch seems to offer better
>>> readability.
>>
>> Two more inputs for maintainers' decision:
>>
>> 1) the original proposed patch fixed one "bug" that is:
>>
>> In function rs6000_debug_reg_global, it tries to print the register class
>> for the register constraint:
>>
>>   fprintf (stderr,
>>            "\n"
>>            "d  reg_class = %s\n"
>>            "f  reg_class = %s\n"
>>            "v  reg_class = %s\n"
>>            "wa reg_class = %s\n"
>>            ...
>>            "\n",
>>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_d]],
>>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_f]],
>>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_v]],
>>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wa]],
>>            ...
>>
>> It uses rs6000_constraints[RS6000_CONSTRAINT_v] which is conditionally
>> set here:
>>
>>   /* Add conditional constraints based on various options, to allow us to
>>      collapse multiple insn patterns.  */
>>   if (TARGET_ALTIVEC)
>>     rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;
>>
>> But the actual register class for register constraint is hardcoded as
>> ALTIVEC_REGS rather than rs6000_constraints[RS6000_CONSTRAINT_v].
> 
> I agree that the information is inaccurate, but it is informal
> debugging output.  And if Altivec is disabled, the value of the
> constraint is irrelevant / garbage.
> 

Yeah, but IMHO it still can confuse new comers at first glance.

>>
>> 2) Bootstrapped and tested one below patch to remove all the code using
>> RS6000_CONSTRAINT_v on powerpc64le-linux-gnu P10 and P9,
>> powerpc64-linux-gnu P8 and P7 with no regressions.
>>
>> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
>> index 37f07fe5358..3652629c5d0 100644
>> --- a/gcc/config/rs6000/rs6000.c
>> +++ b/gcc/config/rs6000/rs6000.c
>> @@ -2320,7 +2320,6 @@ rs6000_debug_reg_global (void)
>>            "\n"
>>            "d  reg_class = %s\n"
>>            "f  reg_class = %s\n"
>> -          "v  reg_class = %s\n"
>>            "wa reg_class = %s\n"
>>            "we reg_class = %s\n"
>>            "wr reg_class = %s\n"
>> @@ -2329,7 +2328,6 @@ rs6000_debug_reg_global (void)
>>            "\n",
>>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_d]],
>>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_f]],
>> -          reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_v]],
>>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wa]],
>>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_we]],
>>            reg_class_names[rs6000_constraints[RS6000_CONSTRAINT_wr]],
>> @@ -2984,11 +2982,6 @@ rs6000_init_hard_regno_mode_ok (bool global_init_p)
>>    if (TARGET_VSX)
>>      rs6000_constraints[RS6000_CONSTRAINT_wa] = VSX_REGS;
>>
>> -  /* Add conditional constraints based on various options, to allow us to
>> -     collapse multiple insn patterns.  */
>> -  if (TARGET_ALTIVEC)
>> -    rs6000_constraints[RS6000_CONSTRAINT_v] = ALTIVEC_REGS;
>> -
>>    if (TARGET_POWERPC64)
>>      {
>>        rs6000_constraints[RS6000_CONSTRAINT_wr] = GENERAL_REGS;
>> diff --git a/gcc/config/rs6000/rs6000.h b/gcc/config/rs6000/rs6000.h
>> index 4d2f88d4218..48323b80eee 100644
>> --- a/gcc/config/rs6000/rs6000.h
>> +++ b/gcc/config/rs6000/rs6000.h
>> @@ -1237,7 +1237,6 @@ extern enum reg_class rs6000_regno_regclass[FIRST_PSEUDO_REGISTER];
>>  enum r6000_reg_class_enum {
>>    RS6000_CONSTRAINT_d,         /* fpr registers for double values */
>>    RS6000_CONSTRAINT_f,         /* fpr registers for single values */
>> -  RS6000_CONSTRAINT_v,         /* Altivec registers */
>>    RS6000_CONSTRAINT_wa,                /* Any VSX register */
>>    RS6000_CONSTRAINT_we,                /* VSX register if ISA 3.0 vector. */
>>    RS6000_CONSTRAINT_wr,                /* GPR register if 64-bit  */
> 
> I would prefer that we not make gratuitous changes to this code, but
> maybe Segher has a different opinion.
> 

Thanks David for the comments!

Hi Segher, what's your preference on this?

BR,
Kewen
Segher Boessenkool Jan. 26, 2022, 5:57 p.m. UTC | #8 
Hi!

On Wed, Jan 26, 2022 at 10:26:45AM +0800, Kewen.Lin wrote:
> on 2022/1/14 上午12:31, David Edelsohn wrote:
> Yeah, but IMHO it still can confuse new comers at first glance.

Yes, or at least cause to read (well, grep) the whole backend and
scratch heads.

> >> 2) Bootstrapped and tested one below patch to remove all the code using
> >> RS6000_CONSTRAINT_v on powerpc64le-linux-gnu P10 and P9,
> >> powerpc64-linux-gnu P8 and P7 with no regressions.

> > I would prefer that we not make gratuitous changes to this code, but
> > maybe Segher has a different opinion.
> 
> Thanks David for the comments!
> 
> Hi Segher, what's your preference on this?

I like your original patch better.  But for stage 1, sorry.

Indeed using ALTIVEC_REGS directly in the define_regiater_constraint
works fine, but it isn't as clear as it could be that is correct.


Segher
Kewen.Lin Jan. 27, 2022, 11:24 a.m. UTC | #9 
on 2022/1/27 上午1:57, Segher Boessenkool wrote:
> Hi!
> 
> On Wed, Jan 26, 2022 at 10:26:45AM +0800, Kewen.Lin wrote:
>> on 2022/1/14 上午12:31, David Edelsohn wrote:
>> Yeah, but IMHO it still can confuse new comers at first glance.
> 
> Yes, or at least cause to read (well, grep) the whole backend and
> scratch heads.
> 
>>>> 2) Bootstrapped and tested one below patch to remove all the code using
>>>> RS6000_CONSTRAINT_v on powerpc64le-linux-gnu P10 and P9,
>>>> powerpc64-linux-gnu P8 and P7 with no regressions.
> 
>>> I would prefer that we not make gratuitous changes to this code, but
>>> maybe Segher has a different opinion.
>>
>> Thanks David for the comments!
>>
>> Hi Segher, what's your preference on this?
> 
> I like your original patch better.  But for stage 1, sorry.
> 

Thanks Segher!  Is it ok to commit it then?  Or I'll repost this once
next stage1 starts.

BR,
Kewen

> Indeed using ALTIVEC_REGS directly in the define_regiater_constraint
> works fine, but it isn't as clear as it could be that is correct.
> 
> 
> Segher
>
Segher Boessenkool Jan. 27, 2022, 12:51 p.m. UTC | #10 
On Thu, Jan 27, 2022 at 07:24:53PM +0800, Kewen.Lin wrote:
> on 2022/1/27 上午1:57, Segher Boessenkool wrote:
> > I like your original patch better.  But for stage 1, sorry.
> 
> Thanks Segher!  Is it ok to commit it then?  Or I'll repost this once
> next stage1 starts.

Either is fine in this case.


Segher
Kewen.Lin May 11, 2022, 8:56 a.m. UTC | #11 
on 2022/1/27 20:51, Segher Boessenkool wrote:
> On Thu, Jan 27, 2022 at 07:24:53PM +0800, Kewen.Lin wrote:
>> on 2022/1/27 上午1:57, Segher Boessenkool wrote:
>>> I like your original patch better.  But for stage 1, sorry.
>>
>> Thanks Segher!  Is it ok to commit it then?  Or I'll repost this once
>> next stage1 starts.
> 
> Either is fine in this case.
> 

Thanks!  Re-tested and pushed in r13-283.

BR,
Kewen
diff mbox series

Patch

diff --git a/gcc/config/rs6000/constraints.md b/gcc/config/rs6000/constraints.md
index a4b05837fa6..c01dcbbc3a3 100644
--- a/gcc/config/rs6000/constraints.md
+++ b/gcc/config/rs6000/constraints.md
@@ -37,7 +37,7 @@  (define_register_constraint "d" "rs6000_constraints[RS6000_CONSTRAINT_d]"
    historically @code{f} was for single-precision and @code{d} was for
    double-precision floating point.")

-(define_register_constraint "v" "ALTIVEC_REGS"
+(define_register_constraint "v" "rs6000_constraints[RS6000_CONSTRAINT_v]"
   "An Altivec vector register (VR), @code{v0}@dots{}@code{v31}.")

 (define_register_constraint "wa" "rs6000_constraints[RS6000_CONSTRAINT_wa]"