rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v]

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

Commit Message

Jonathan Wakely via Gcc-patches Jan. 13, 2022, 1:55 a.m.
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

Jonathan Wakely via Gcc-patches Jan. 13, 2022, 3:07 a.m. | #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
Jonathan Wakely via Gcc-patches Jan. 13, 2022, 3:38 a.m. | #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

>
Jonathan Wakely via Gcc-patches Jan. 13, 2022, 3:44 a.m. | #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
Jonathan Wakely via Gcc-patches Jan. 13, 2022, 3:56 a.m. | #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

>
Jonathan Wakely via Gcc-patches Jan. 13, 2022, 12:28 p.m. | #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
Jonathan Wakely via Gcc-patches Jan. 13, 2022, 4:31 p.m. | #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

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