public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Hongyu Wang <wwwhhhyyy333@gmail.com>
To: Jakub Jelinek <jakub@redhat.com>
Cc: Hongyu Wang <hongyu.wang@intel.com>,
	gcc-patches@gcc.gnu.org, hongtao.liu@intel.com,  hubicka@ucw.cz
Subject: Re: [PATCH 06/13] [APX EGPR] Map reg/mem constraints in inline asm to non-EGPR constraint.
Date: Fri, 1 Sep 2023 17:04:15 +0800	[thread overview]
Message-ID: <CA+OydWmGE=vJTfUsD6N5Oy_woM6aPB69FCm+JKvhFpt71TnNCQ@mail.gmail.com> (raw)
In-Reply-To: <ZPBakcmZY/MIX/Su@tucnak>

Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> 于2023年8月31日周四 17:18写道:
>
> On Thu, Aug 31, 2023 at 04:20:17PM +0800, Hongyu Wang via Gcc-patches wrote:
> > From: Kong Lingling <lingling.kong@intel.com>
> >
> > In inline asm, we do not know if the insn can use EGPR, so disable EGPR
> > usage by default from mapping the common reg/mem constraint to non-EGPR
> > constraints. Use a flag mapx-inline-asm-use-gpr32 to enable EGPR usage
> > for inline asm.
> >
> > gcc/ChangeLog:
> >
> >       * config/i386/i386.cc (INCLUDE_STRING): Add include for
> >       ix86_md_asm_adjust.
> >       (ix86_md_asm_adjust): When APX EGPR enabled without specifying the
> >       target option, map reg/mem constraints to non-EGPR constraints.
> >       * config/i386/i386.opt: Add option mapx-inline-asm-use-gpr32.
> >
> > gcc/testsuite/ChangeLog:
> >
> >       * gcc.target/i386/apx-inline-gpr-norex2.c: New test.
> > ---
> >  gcc/config/i386/i386.cc                       |  44 +++++++
> >  gcc/config/i386/i386.opt                      |   5 +
> >  .../gcc.target/i386/apx-inline-gpr-norex2.c   | 107 ++++++++++++++++++
> >  3 files changed, 156 insertions(+)
> >  create mode 100644 gcc/testsuite/gcc.target/i386/apx-inline-gpr-norex2.c
> >
> > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
> > index d26d9ab0d9d..9460ebbfda4 100644
> > --- a/gcc/config/i386/i386.cc
> > +++ b/gcc/config/i386/i386.cc
> > @@ -17,6 +17,7 @@ You should have received a copy of the GNU General Public License
> >  along with GCC; see the file COPYING3.  If not see
> >  <http://www.gnu.org/licenses/>.  */
> >
> > +#define INCLUDE_STRING
> >  #define IN_TARGET_CODE 1
> >
> >  #include "config.h"
> > @@ -23077,6 +23078,49 @@ ix86_md_asm_adjust (vec<rtx> &outputs, vec<rtx> & /*inputs*/,
> >    bool saw_asm_flag = false;
> >
> >    start_sequence ();
> > +  /* TODO: Here we just mapped the general r/m constraints to non-EGPR
> > +   constraints, will eventually map all the usable constraints in the future. */
>
> I think there should be some constraint which explicitly has all the 32
> GPRs, like there is one for just all 16 GPRs (h), so that regardless of
> -mapx-inline-asm-use-gpr32 one can be explicit what the inline asm wants.
>

Yes, we will add new register constraints. For memory constraints it requires
some special handling in ix86_memory_address_use_extended_reg_class_p

> Also, what about the "g" constraint?  Shouldn't there be another for "g"
> without r16..r31?  What about the various other memory
> constraints ("<", "o", ...)?

We will support fully mapping of all common constraints, with refining
of current
mapping in the V2 patch.

>
> > +  if (TARGET_APX_EGPR && !ix86_apx_inline_asm_use_gpr32)
> > +    {
> > +      /* Map "r" constraint in inline asm to "h" that disallows r16-r31
> > +      and replace only r, exclude Br and Yr.  */
> > +      for (unsigned i = 0; i < constraints.length (); i++)
> > +     {
> > +       std::string *s = new std::string (constraints[i]);
>
> Doesn't this leak memory (all the time)?
> I must say I don't really understand why you need to use std::string here,
> but certainly it shouldn't leak.

std::string just makes the code shorter than using str functions. Current code
will be completely refactored with supporting more mapping of constraints.

>
> > +       size_t pos = s->find ('r');
> > +       while (pos != std::string::npos)
> > +         {
> > +           if (pos > 0
> > +               && (s->at (pos - 1) == 'Y' || s->at (pos - 1) == 'B'))
> > +             pos = s->find ('r', pos + 1);
> > +           else
> > +             {
> > +               s->replace (pos, 1, "h");
> > +               constraints[i] = (const char*) s->c_str ();
>
> Formatting (space before *).  The usual way for constraints is ggc_strdup on
> some string in a buffer.  Also, one could have several copies or r (or m, memory (doesn't
> that appear just in clobbers?  And that doesn't look like something that
> should be replaced), Bm, e.g. in various alternatives.  So, you
> need to change them all, not just the first hit.  "r,r,r,m" and the like.
> Normally, one would simply walk the constraint string, parsing the special
> letters (+, =, & etc.) and single letter constraints and 2 letter
> constraints using CONSTRAINT_LEN macro (tons of examples in GCC sources).
> Either do it in 2 passes, first one counts how long constraint string one
> will need after the adjustments (and whether to adjust something at all),
> then if needed XALLOCAVEC it and adjust in there, or say use a
> auto_vec<char, 32> for
> it.

Thanks for your guidance. Previously we thought constraints[i] was a splitted
simple constraint, but clearly it is not.
We will refer to an existing example of this and rewrite current one.

>
> > +               break;
> > +             }
> > +         }
> > +     }
> > +      /* Also map "m/memory/Bm" constraint that may use GPR32, replace them with
> > +      "Bt/Bt/BT".  */
> > +      for (unsigned i = 0; i < constraints.length (); i++)
> > +     {
> > +       std::string *s = new std::string (constraints[i]);
> > +       size_t pos = s->find ("m");
> > +       size_t pos2 = s->find ("memory");
> > +       if (pos != std::string::npos)
> > +         {
> > +           if (pos > 0 && (s->at (pos - 1) == 'B'))
> > +               s->replace (pos - 1, 2, "BT");
> > +           else if (pos2 != std::string::npos)
> > +               s->replace (pos, 6, "Bt");
> > +           else
> > +               s->replace (pos, 1, "Bt");
>
> Formatting, the s->replace calls are indented too much.
>
>         Jakub
>

  parent reply	other threads:[~2023-09-01  9:10 UTC|newest]

Thread overview: 47+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-31  8:20 [PATCH 00/13] [RFC] Support Intel APX EGPR Hongyu Wang
2023-08-31  8:20 ` [PATCH 01/13] [APX EGPR] middle-end: Add insn argument to base_reg_class Hongyu Wang
2023-08-31 10:15   ` Uros Bizjak
2023-09-01  9:07     ` Hongyu Wang
2023-09-06 19:43       ` Vladimir Makarov
2023-09-07  6:23         ` Uros Bizjak
2023-09-07 12:13           ` Vladimir Makarov
2023-09-08 17:03   ` Vladimir Makarov
2023-09-10  4:49     ` Hongyu Wang
2023-09-14 12:09       ` Vladimir Makarov
2023-08-31  8:20 ` [PATCH 02/13] [APX EGPR] middle-end: Add index_reg_class with insn argument Hongyu Wang
2023-08-31  8:20 ` [PATCH 03/13] [APX_EGPR] Initial support for APX_F Hongyu Wang
2023-08-31  8:20 ` [PATCH 04/13] [APX EGPR] Add 16 new integer general purpose registers Hongyu Wang
2023-08-31  8:20 ` [PATCH 05/13] [APX EGPR] Add register and memory constraints that disallow EGPR Hongyu Wang
2023-08-31  8:20 ` [PATCH 06/13] [APX EGPR] Map reg/mem constraints in inline asm to non-EGPR constraint Hongyu Wang
2023-08-31  9:17   ` Jakub Jelinek
2023-08-31 10:00     ` Uros Bizjak
2023-09-01  9:04       ` Hongyu Wang
2023-09-01  9:38         ` Uros Bizjak
2023-09-01 10:35           ` Hongtao Liu
2023-09-01 11:27             ` Uros Bizjak
2023-09-04  0:28               ` Hongtao Liu
2023-09-04  8:57                 ` Uros Bizjak
2023-09-04  9:10                   ` Hongtao Liu
2023-09-01 11:03       ` Richard Sandiford
2023-09-04  1:03         ` Hongtao Liu
2023-09-01  9:04     ` Hongyu Wang [this message]
2023-08-31  8:20 ` [PATCH 07/13] [APX EGPR] Add backend hook for base_reg_class/index_reg_class Hongyu Wang
2023-08-31  8:20 ` [PATCH 08/13] [APX EGPR] Handle GPR16 only vector move insns Hongyu Wang
2023-08-31  9:43   ` Jakub Jelinek
2023-09-01  9:07     ` Hongyu Wang
2023-09-01  9:20       ` Jakub Jelinek
2023-09-01 11:34         ` Hongyu Wang
2023-09-01 11:41           ` Jakub Jelinek
2023-08-31  8:20 ` [PATCH 09/13] [APX EGPR] Handle legacy insn that only support GPR16 (1/5) Hongyu Wang
2023-08-31 10:06   ` Uros Bizjak
2023-08-31  8:20 ` [PATCH 10/13] [APX EGPR] Handle legacy insns that only support GPR16 (2/5) Hongyu Wang
2023-08-31  8:20 ` [PATCH 11/13] [APX EGPR] Handle legacy insns that only support GPR16 (3/5) Hongyu Wang
2023-08-31  9:26   ` Richard Biener
2023-08-31  9:28     ` Richard Biener
2023-09-01  9:03       ` Hongyu Wang
2023-09-01 10:38       ` Hongtao Liu
2023-08-31  9:31     ` Jakub Jelinek
2023-08-31  8:20 ` [PATCH 12/13] [APX_EGPR] Handle legacy insns that only support GPR16 (4/5) Hongyu Wang
2023-08-31  8:20 ` [PATCH 13/13] [APX EGPR] Handle vex insns that only support GPR16 (5/5) Hongyu Wang
2023-08-31  9:19 ` [PATCH 00/13] [RFC] Support Intel APX EGPR Richard Biener
2023-09-01  8:55   ` Hongyu Wang

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='CA+OydWmGE=vJTfUsD6N5Oy_woM6aPB69FCm+JKvhFpt71TnNCQ@mail.gmail.com' \
    --to=wwwhhhyyy333@gmail.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=hongtao.liu@intel.com \
    --cc=hongyu.wang@intel.com \
    --cc=hubicka@ucw.cz \
    --cc=jakub@redhat.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).