Re: [PATCH] Make sure we're playing with integral modes before call extract_integral_bit_field.

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Aug 6, 2021 at 11:05 AM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Richard Biener via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> > On Fri, Aug 6, 2021 at 5:32 AM liuhongt <hongtao.liu@intel.com> wrote:
> >>
> >> Hi:
> >> ---
> >> OK, I think sth is amiss here upthread.  insv/extv do look like they
> >> are designed
> >> to work on integer modes (but docs do not say anything about this here).
> >> In fact the caller of extract_bit_field_using_extv is named
> >> extract_integral_bit_field.  Of course nothing seems to check what kind of
> >> modes we're dealing with, but we're for example happily doing
> >> expand_shift in 'mode'.  In the extract_integral_bit_field call 'mode' is
> >> some integer mode and op0 is HFmode?  From the above I get it's
> >> the other way around?  In that case we should wrap the
> >> call to extract_integral_bit_field, extracting in an integer mode with the
> >> same size as 'mode' and then converting the result as (subreg:HF (reg:HI ...)).
> >> ---
> >>   This is a separate patch as a follow up of upper comments.
> >>
> >> gcc/ChangeLog:
> >>
> >>         * expmed.c (extract_bit_field_1): Wrap the call to
> >>         extract_integral_bit_field, extracting in an integer mode with
> >>         the same size as 'tmode' and then converting the result
> >>         as (subreg:tmode (reg:imode)).
> >>
> >> gcc/testsuite/ChangeLog:
> >>         * gcc.target/i386/float16-5.c: New test.
> >> ---
> >>  gcc/expmed.c                              | 19 +++++++++++++++++++
> >>  gcc/testsuite/gcc.target/i386/float16-5.c | 12 ++++++++++++
> >>  2 files changed, 31 insertions(+)
> >>  create mode 100644 gcc/testsuite/gcc.target/i386/float16-5.c
> >>
> >> diff --git a/gcc/expmed.c b/gcc/expmed.c
> >> index 3143f38e057..72790693ef0 100644
> >> --- a/gcc/expmed.c
> >> +++ b/gcc/expmed.c
> >> @@ -1850,6 +1850,25 @@ extract_bit_field_1 (rtx str_rtx, poly_uint64 bitsize, poly_uint64 bitnum,
> >>        op0_mode = opt_scalar_int_mode ();
> >>      }
> >>
> >> +  /* Make sure we are playing with integral modes.  Pun with subregs
> >> +     if we aren't. When tmode is HFmode, op0 is SImode, there will be ICE
> >> +     in extract_integral_bit_field.  */
> >> +  if (int_mode_for_mode (tmode).exists (&imode)
> >
> > check !INTEGRAL_MODE_P (tmode) before, that should be slightly
> > cheaper.  Then imode should always be != tmode.  Maybe
> > even GET_MDOE_CLASS (tmode) != MODE_INT since I'm not sure
> > how it behaves for composite modes.
> >
> > Of course the least surprises would happen when we restrict this
> > to FLOAT_MODE_P (tmode).
> >
> > Richard - any preferences?
>
> If the bug is that extract_integral_bit_field is being called with
> a non-integral mode parameter, then it looks odd that we can still
> fall through to it without an integral mode (when exists is false).
>
> If calling extract_integral_bit_field without an integral mode is
> a bug then I think we should have:
>
>   int_mode_for_mode (mode).require ()
>
> whenever mode is not already SCALAR_INT_MODE_P/is_a<scalar_int_mode>.
> Ideally we'd make the mode parameter scalar_int_mode too.
>
> extract_integral_bit_field currently has:
>
>   /* Find a correspondingly-sized integer field, so we can apply
>      shifts and masks to it.  */
>   scalar_int_mode int_mode;
>   if (!int_mode_for_mode (tmode).exists (&int_mode))
>     /* If this fails, we should probably push op0 out to memory and then
>        do a load.  */
>     int_mode = int_mode_for_mode (mode).require ();
>
> which would seem to be redundant after this change.

I'm not sure what exactly the bug is, but extract_integral_bit_field ends
up creating a lowpart subreg that's not allowed and that ICEs (and I
can't see a way to check beforehand).  So it seems to me at least
part of that function doesn't expect non-integral extraction modes.

But who knows - the code is older than I am (OK, not, but older than
my involvment in GCC ;))

Richard.

> >> +      && imode != tmode
> >> +      && imode != GET_MODE (op0))
> >> +    {
> >> +      rtx ret = extract_integral_bit_field (op0, op0_mode,
> >> +                                           bitsize.to_constant (),
> >> +                                           bitnum.to_constant (), unsignedp,
> >> +                                           NULL, imode, imode,
> >> +                                           reverse, fallback_p);
> >> +      gcc_assert (ret);
> >> +
> >> +      if (!REG_P (ret))
> >> +       ret = force_reg (imode, ret);
> >> +      return gen_lowpart_SUBREG (tmode, ret);
> >> +    }
> >> +
> >>    /* It's possible we'll need to handle other cases here for
> >>       polynomial bitnum and bitsize.  */
>
> Minor nit, but since the code is using to_constant, it should go after
> this comment rather than before it.
>
> Thanks,
> Richard
>
> >>
> >> diff --git a/gcc/testsuite/gcc.target/i386/float16-5.c b/gcc/testsuite/gcc.target/i386/float16-5.c
> >> new file mode 100644
> >> index 00000000000..ebc0af1490b
> >> --- /dev/null
> >> +++ b/gcc/testsuite/gcc.target/i386/float16-5.c
> >> @@ -0,0 +1,12 @@
> >> +/* { dg-do compile } */
> >> +/* { dg-options "-msse2 -O2" } */
> >> +_Float16
> >> +foo (int a)
> >> +{
> >> +  union {
> >> +    int a;
> >> +    _Float16 b;
> >> +  }c;
> >> +  c.a = a;
> >> +  return c.b;
> >> +}
> >> --
> >> 2.27.0
> >>