Re: [PATCH 06/13] [APX EGPR] Map reg/mem constraints in inline asm to non-EGPR constraint.

[Uros Bizjak <ubizjak@gmail.com>]

On Fri, Sep 1, 2023 at 12:36 PM Hongtao Liu <crazylht@gmail.com> wrote:
>
> On Fri, Sep 1, 2023 at 5:38 PM Uros Bizjak via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
> >
> > On Fri, Sep 1, 2023 at 11:10 AM Hongyu Wang <wwwhhhyyy333@gmail.com> wrote:
> > >
> > > Uros Bizjak via Gcc-patches <gcc-patches@gcc.gnu.org> 于2023年8月31日周四 18:01写道：
> > > >
> > > > On Thu, Aug 31, 2023 at 11:18 AM Jakub Jelinek via Gcc-patches
> > > > <gcc-patches@gcc.gnu.org> wrote:
> > > > >
> > > > > 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.
> > > > >
> > > > > 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.

Uros.