From: "Kewen.Lin" <linkw@linux.ibm.com>
To: David Edelsohn <dje.gcc@gmail.com>,
Segher Boessenkool <segher@kernel.crashing.org>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>,
Bill Schmidt <wschmidt@linux.ibm.com>
Subject: Re: [PATCH] rs6000: Fix constraint v with rs6000_constraints[RS6000_CONSTRAINT_v]
Date: Wed, 26 Jan 2022 10:26:45 +0800 [thread overview]
Message-ID: <946895dd-7938-12ce-2f59-cbceb3bef49a@linux.ibm.com> (raw)
In-Reply-To: <CAGWvnyk_4XaR_6oFi479VAhDpz9wpGMaLep0_JnGhKEsN6ZUYg@mail.gmail.com>
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
next prev parent reply other threads:[~2022-01-26 2:26 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-13 1:55 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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=946895dd-7938-12ce-2f59-cbceb3bef49a@linux.ibm.com \
--to=linkw@linux.ibm.com \
--cc=dje.gcc@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=segher@kernel.crashing.org \
--cc=wschmidt@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).