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