RE: [PATCH][i386]: Update ix86_can_change_mode_class target hook to accept QImode conversions

[Tamar Christina <Tamar.Christina@arm.com>]

> -----Original Message-----
> From: Hongtao Liu <crazylht@gmail.com>
> Sent: Monday, November 14, 2022 2:14 AM
> To: Tamar Christina <Tamar.Christina@arm.com>
> Cc: gcc-patches@gcc.gnu.org; nd <nd@arm.com>; hubicka@ucw.cz;
> ubizjak@gmail.com; kirill.yukhin@gmail.com; hongtao.liu@intel.com
> Subject: Re: [PATCH][i386]: Update ix86_can_change_mode_class target
> hook to accept QImode conversions
> 
> On Fri, Nov 11, 2022 at 10:47 PM Tamar Christina via Gcc-patches <gcc-
> patches@gcc.gnu.org> wrote:
> >
> > Hi All,
> >
> > The current i386 implementation of the
> TARGET_CAN_CHANGE_MODE_CLASS is
> > currently not useful before re-alloc.
> >
> > In particular before regalloc optimization passes query the hook using
> > ALL_REGS, but because of the
> >
> >   if (MAYBE_FLOAT_CLASS_P (regclass))
> >       return false;
> >
> > The hook returns false for all modes, even integer ones because
> > ALL_REGS overlaps with floating point regs.
> >
> > The vector permute fallback cases used to unconditionally convert
> > vector integer permutes to vector QImode ones as a fallback plan.
> > This is incorrect and can result in incorrect code if the target doesn't
> support this conversion.
> >
> > To fix this some more checks were added, however that ended up
> > introducing ICEs in the i386 backend because e.g. the hook would
> > reject conversions between modes like V2TImode and V32QImode.
> >
> > My understanding is that for x87 we don't want to allow floating point
> > conversions, but integers are fine.  So I have modified the check such
> > that it also checks the modes, not just the register class groups.
> >
> > The second part of the code is needed because now that integer modes
> > aren't uniformly rejected the i386 backend trigger further
> > optimizations.  However the backend lacks instructions to deal with
> > canonical RTL representations of certain instructions.  for instance
> > the back-end seems to prefer vec_select 0 instead of subregs.
> >
> > So to prevent the canonicalization I reject integer modes when the
> > sizes of to and from don't match and when we would have exited with
> false previously.
> >
> > This fixes all the ICEs and codegen regressions, but perhaps an x86
> > maintainer should take a deeper look at this hook implementation.
> >
> > Bootstrapped Regtested on x86_64-pc-linux-gnu and no issues.
> >
> > Ok for master?
> >
> > Thanks,
> > Tamar
> >
> > gcc/ChangeLog:
> >
> >         * config/i386/i386.cc (ix86_can_change_mode_class): Update the
> target
> >         hook.
> >
> > --- inline copy of patch --
> > diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc index
> >
> c4d0e36e9c0a2256f5dde1f4dc021c0328aa0cba..477dd007ea80272680751b61e
> 35c
> > c3eec79b66c3 100644
> > --- a/gcc/config/i386/i386.cc
> > +++ b/gcc/config/i386/i386.cc
> > @@ -19682,7 +19682,15 @@ ix86_can_change_mode_class
> (machine_mode
> > from, machine_mode to,
> >
> >    /* x87 registers can't do subreg at all, as all values are reformatted
> >       to extended precision.  */
> > -  if (MAYBE_FLOAT_CLASS_P (regclass))
> > +  if (MAYBE_FLOAT_CLASS_P (regclass)
> > +      && VALID_FP_MODE_P (from)
> > +      && VALID_FP_MODE_P (to))
> > +    return false;
> This change looks reasonable since only VALID_FP_MODE_P will be allocated
> to FLOAT_CLASS.
> > +
> > +  /* Reject integer modes if the sizes aren't the same.  It would have
> > +     normally exited above.  */
> > +  if (MAYBE_FLOAT_CLASS_P (regclass)
> > +      && GET_MODE_SIZE (from) != GET_MODE_SIZE (to))
> >      return false;
> Do you have a case(or a patch so I can reproduce the regression
> myself) to indicate the regression, so I can have a deep look.

Yes that's this one https://gcc.gnu.org/pipermail/gcc-patches/2022-November/605756.html

Cheers,
Tamar

> >
> >    if (MAYBE_SSE_CLASS_P (regclass) || MAYBE_MMX_CLASS_P (regclass))
> >
> >
> >
> >
> > --
> 
> 
> 
> --
> BR,
> Hongtao