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