On Fri, Aug 6, 2021 at 6:54 AM Hongtao Liu via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> 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)
> ;
Yeah, as said all this verification looks dubious. This one especially so.
> 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
