public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Uros Bizjak <ubizjak@gmail.com>
To: Hongtao Liu <crazylht@gmail.com>
Cc: Hongyu Wang <wwwhhhyyy333@gmail.com>,
	Jakub Jelinek <jakub@redhat.com>,
	 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: Mon, 4 Sep 2023 10:57:29 +0200	[thread overview]
Message-ID: <CAFULd4YF=c_+FGcijaCeFo3SnfdqeOiCKmB0GYQ_Z07Oo_ZDTQ@mail.gmail.com> (raw)
In-Reply-To: <CAMZc-bzaW3ROjVbBGL_zXHRhpTTx3=zfzJfCCa0vwWPXB53cFA@mail.gmail.com>

On Mon, Sep 4, 2023 at 2:28 AM Hongtao Liu <crazylht@gmail.com> wrote:

> > > > > > > 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.
> > > > > > >
> > > > > > > Also, what about the "g" constraint?  Shouldn't there be another for "g"
> > > > > > > without r16..r31?  What about the various other memory
> > > > > > > constraints ("<", "o", ...)?
> > > > > >
> > > > > > I think we should leave all existing constraints as they are, so "r"
> > > > > > covers only GPR16, "m" and "o" to only use GPR16. We can then
> > > > > > introduce "h" to instructions that have the ability to handle EGPR.
> > > > > > This would be somehow similar to the SSE -> AVX512F transition, where
> > > > > > we still have "x" for SSE16 and "v" was introduced as a separate
> > > > > > register class for EVEX SSE registers. This way, asm will be
> > > > > > compatible, when "r", "m", "o" and "g" are used. The new memory
> > > > > > constraint "Bt", should allow new registers, and should be added to
> > > > > > the constraint string as a separate constraint, and conditionally
> > > > > > enabled by relevant "isa" (AKA "enabled") attribute.
> > > > >
> > > > > The extended constraint can work for registers, but for memory it is more
> > > > > complicated.
> > > >
> > > > Yes, unfortunately. The compiler assumes that an unchangeable register
> > > > class is used for BASE/INDEX registers. I have hit this limitation
> > > > when trying to implement memory support for instructions involving
> > > > 8-bit high registers (%ah, %bh, %ch, %dh), which do not support REX
> > > > registers, also inside memory operand. (You can see the "hack" in e.g.
> > > > *extzvqi_mem_rex64" and corresponding peephole2 with the original
> > > > *extzvqi pattern). I am aware that dynamic insn-dependent BASE/INDEX
> > > > register class is the major limitation in the compiler, so perhaps the
> > > > strategy on how to override this limitation should be discussed with
> > > > the register allocator author first. Perhaps adding an insn attribute
> > > > to insn RTX pattern to specify different BASE/INDEX register sets can
> > > > be a better solution than passing insn RTX to the register allocator.
> > > >
> > > > The above idea still does not solve the asm problem on how to select
> > > > correct BASE/INDEX register set for memory operands.
> > > The current approach disables gpr32 for memory operand in asm_operand
> > > by default. but can be turned on by options
> > > ix86_apx_inline_asm_use_gpr32(users need to guarantee the instruction
> > > supports gpr32).
> > > Only ~ 5% of total instructions don't support gpr32, reversed approach
> > > only gonna get more complicated.
> >
> > I'm not referring to the reversed approach, just want to point out
> > that the same approach as you proposed w.r.t. to memory operand can be
> > achieved using some named insn attribute that would affect BASE/INDEX
> > register class selection. The attribute could default to gpr32 with
> > APX, unless the insn specific attribute has e.g. nogpr32 value. See
> > for example how "enabled" and "preferred_for_*" attributes are used.
> > Perhaps this new attribute can also be applied to separate
> > alternatives.
> Yes, for xop/fma4/3dnow instructions, I think we can use isa attr like
> (define_attr "gpr32" "0, 1"
>   (cond [(eq_attr "isa" "fma4")
>            (const_string "0")]
>       (const_string "1")))

Just a nit, can the member be named "map0" and "map1"? The code will
then look like:

if (get_attr_gpr32 (insn) == GPR32_MAP0) ...

instead of:

if (get_attr_gpr32 (insn) == GPR32_0) ...

> But still, we need to adjust memory constraints in the pattern.

I guess the gpr32 property is the same for all alternatives of the
insn pattern. In this case,  "m" "g" and "a" constraints could remain
as they are, the final register class will be adjusted (by some target
hook?) based on the value of gpr32 attribute.

> Ideally, gcc includes encoding information for every instruction,
> (.i.e. map0/map1), so that we can determine the attribute value of
> gpr32 directly from this information.

I think the right tool for this is attribute infrastructure of insn
patterns. We can set the default, set precise value of the insns, or
calculate attribute from some other attribute in a quite flexible way.
Other than that, adjusting BASE/INDEX register class of the RA pass is
the infrastructure change, but perhaps similar to the one you
proposed.

Uros.

  reply	other threads:[~2023-09-04  8:57 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 [this message]
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
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='CAFULd4YF=c_+FGcijaCeFo3SnfdqeOiCKmB0GYQ_Z07Oo_ZDTQ@mail.gmail.com' \
    --to=ubizjak@gmail.com \
    --cc=crazylht@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 \
    --cc=wwwhhhyyy333@gmail.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).