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

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..477dd007ea80272680751b61e35cc3eec79b66c3 100644




--
diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
index c4d0e36e9c0a2256f5dde1f4dc021c0328aa0cba..477dd007ea80272680751b61e35cc3eec79b66c3 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;
+
+  /* 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;
 
   if (MAYBE_SSE_CLASS_P (regclass) || MAYBE_MMX_CLASS_P (regclass))

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..477dd007ea80272680751b61e35cc3eec79b66c3 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.
>
>    if (MAYBE_SSE_CLASS_P (regclass) || MAYBE_MMX_CLASS_P (regclass))
>
>
>
>
> --

> -----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

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.
If the canonical form of the lower half of the vector in rtl is subreg
instead of vec_select, the x86 backend needs to update the pattern
accordingly to avoid diverge.
I can reproduce those failures when adding VALID_FP_MODE_P in
targetm.can_change_mode_class.
>
> 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..477dd007ea80272680751b61e35cc3eec79b66c3 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;
> +
> +  /* 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;
>
>    if (MAYBE_SSE_CLASS_P (regclass) || MAYBE_MMX_CLASS_P (regclass))
>
>
>
>
> --

On Mon, Nov 14, 2022 at 10:14 AM Hongtao Liu <crazylht@gmail.com> wrote:
>
> 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
This should not be right, we still want something like (subreg:SI (reg:V4SI))
> > 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..477dd007ea80272680751b61e35cc3eec79b66c3 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.
This is not right, we still want something like (subreg:SI
(reg:V4SI)). I'm trying to adjust all those vec_select to subregs, and
it may take some time. We also need to adjust modes_tieable_p to
prevent high rtx_cost for subreg which will prevent insns being
combined. x86 is conservative on both can_change_mode_class and
modes_tieable_p.
I'm a bit worried that these changes are a bit big for stage3, let me
try it first.
> >
> >    if (MAYBE_SSE_CLASS_P (regclass) || MAYBE_MMX_CLASS_P (regclass))
> >
> >
> >
> >
> > --
>
>
>
> --
> BR,
> Hongtao