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
