on 2022/1/14 上午12:31, David Edelsohn wrote: > On Thu, Jan 13, 2022 at 7:28 AM Kewen.Lin <li...@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 <li...@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 <li...@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