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

[Hongtao Liu]

On Fri, Aug 6, 2021 at 11:44 AM Andrew Pinski via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Thu, Aug 5, 2021 at 8:33 PM liuhongt via Gcc-patches
> <gcc-patches@gcc.gnu.org> 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 seems related to https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93235 .
> I wonder why the fix for that did not help here.
>
aarch64 didn't hit gcc_assert with my testcase, and I debugged it to
figure out why.

in gimple level, both x86 and aarch64 is the same with
_3 = BIT_FIELD_REF <a_2(D), 16, 0>;

and they all goes into
extract_bit_field_using_extv

The difference is aarch64 has ext_mode as DImode, but x86 has ext_mode
as SImode.
with ext_mode as DImode and target as (reg:HF 94), aarch64 doesn't hit
gcc_assert in
 gen_lowpart (ext_mode, target)

since validate_subreg allow (subreg:DI (reg:HF)), but disallow
(subreg:SI (reg:HF)).

  /* ??? This should not be here.  Temporarily continue to allow word_mode
     subregs of anything.  The most common offender is (subreg:SI (reg:DF)).
     Generally, backends are doing something sketchy but it'll take time to
     fix them all.  */
  if (omode == word_mode)
    ;

ext_mode is assigned from extv->field mode which is initialized in
get_best_reg_extraction_insn.
get_best_reg_extraction_insn will finally call
get_optab_extraction_insn and find
aarch64 doesn't have CODE_FOR_extzvsi but x86 has.

That's why aarch64 has ext_mode as DImode and x86 SImode.

> Thanks,
> Andrew Pinski
>
> > ---
> >   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)
> > +      && 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.  */
> >
> > 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
> >



-- 
BR,
Hongtao