* [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] @ 2022-01-13 1:55 Kewen.Lin 2022-01-13 3:07 ` David Edelsohn 0 siblings, 1 reply; 12+ messages in thread From: Kewen.Lin @ 2022-01-13 1:55 UTC (permalink / raw) To: GCC Patches; +Cc: Segher Boessenkool, David Edelsohn, Bill Schmidt 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(-) 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]" -- 2.27.0 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] 2022-01-13 1:55 [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] Kewen.Lin @ 2022-01-13 3:07 ` David Edelsohn 2022-01-13 3:38 ` Kewen.Lin 0 siblings, 1 reply; 12+ messages in thread From: David Edelsohn @ 2022-01-13 3:07 UTC (permalink / raw) To: Kewen.Lin; +Cc: GCC Patches, Segher Boessenkool, Bill Schmidt 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] 2022-01-13 3:07 ` David Edelsohn @ 2022-01-13 3:38 ` Kewen.Lin 2022-01-13 3:44 ` David Edelsohn 0 siblings, 1 reply; 12+ messages in thread From: Kewen.Lin @ 2022-01-13 3:38 UTC (permalink / raw) To: David Edelsohn; +Cc: GCC Patches, Segher Boessenkool, Bill Schmidt 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] 2022-01-13 3:38 ` Kewen.Lin @ 2022-01-13 3:44 ` David Edelsohn 2022-01-13 3:56 ` Kewen.Lin 0 siblings, 1 reply; 12+ messages in thread From: David Edelsohn @ 2022-01-13 3:44 UTC (permalink / raw) To: Kewen.Lin; +Cc: GCC Patches, Segher Boessenkool, Bill Schmidt 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] 2022-01-13 3:44 ` David Edelsohn @ 2022-01-13 3:56 ` Kewen.Lin 2022-01-13 12:28 ` Kewen.Lin 0 siblings, 1 reply; 12+ messages in thread From: Kewen.Lin @ 2022-01-13 3:56 UTC (permalink / raw) To: David Edelsohn; +Cc: GCC Patches, Segher Boessenkool, Bill Schmidt 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] 2022-01-13 3:56 ` Kewen.Lin @ 2022-01-13 12:28 ` Kewen.Lin 2022-01-13 16:31 ` David Edelsohn 0 siblings, 1 reply; 12+ messages in thread From: Kewen.Lin @ 2022-01-13 12:28 UTC (permalink / raw) To: GCC Patches; +Cc: Bill Schmidt, Segher Boessenkool, David Edelsohn 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] 2022-01-13 12:28 ` Kewen.Lin @ 2022-01-13 16:31 ` David Edelsohn 2022-01-26 2:26 ` Kewen.Lin 0 siblings, 1 reply; 12+ messages in thread From: David Edelsohn @ 2022-01-13 16:31 UTC (permalink / raw) To: Kewen.Lin; +Cc: GCC Patches, Bill Schmidt, Segher Boessenkool 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] 2022-01-13 16:31 ` David Edelsohn @ 2022-01-26 2:26 ` Kewen.Lin 2022-01-26 17:57 ` Segher Boessenkool 0 siblings, 1 reply; 12+ messages in thread From: Kewen.Lin @ 2022-01-26 2:26 UTC (permalink / raw) To: David Edelsohn, Segher Boessenkool; +Cc: GCC Patches, Bill Schmidt 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] 2022-01-26 2:26 ` Kewen.Lin @ 2022-01-26 17:57 ` Segher Boessenkool 2022-01-27 11:24 ` Kewen.Lin 0 siblings, 1 reply; 12+ messages in thread From: Segher Boessenkool @ 2022-01-26 17:57 UTC (permalink / raw) To: Kewen.Lin; +Cc: David Edelsohn, GCC Patches, Bill Schmidt 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] 2022-01-26 17:57 ` Segher Boessenkool @ 2022-01-27 11:24 ` Kewen.Lin 2022-01-27 12:51 ` Segher Boessenkool 0 siblings, 1 reply; 12+ messages in thread From: Kewen.Lin @ 2022-01-27 11:24 UTC (permalink / raw) To: Segher Boessenkool; +Cc: David Edelsohn, GCC Patches, Bill Schmidt 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] 2022-01-27 11:24 ` Kewen.Lin @ 2022-01-27 12:51 ` Segher Boessenkool 2022-05-11 8:56 ` Kewen.Lin 0 siblings, 1 reply; 12+ messages in thread From: Segher Boessenkool @ 2022-01-27 12:51 UTC (permalink / raw) To: Kewen.Lin; +Cc: David Edelsohn, GCC Patches, Bill Schmidt 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] 2022-01-27 12:51 ` Segher Boessenkool @ 2022-05-11 8:56 ` Kewen.Lin 0 siblings, 0 replies; 12+ messages in thread From: Kewen.Lin @ 2022-05-11 8:56 UTC (permalink / raw) To: Segher Boessenkool; +Cc: David Edelsohn, GCC Patches, Bill Schmidt 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-05-11 8:56 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-13 1:55 [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v] Kewen.Lin 2022-01-13 3:07 ` David Edelsohn 2022-01-13 3:38 ` Kewen.Lin 2022-01-13 3:44 ` David Edelsohn 2022-01-13 3:56 ` Kewen.Lin 2022-01-13 12:28 ` Kewen.Lin 2022-01-13 16:31 ` David Edelsohn 2022-01-26 2:26 ` Kewen.Lin 2022-01-26 17:57 ` Segher Boessenkool 2022-01-27 11:24 ` Kewen.Lin 2022-01-27 12:51 ` Segher Boessenkool 2022-05-11 8:56 ` Kewen.Lin
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).