On Fri, Aug 6, 2021 at 11:05 AM Richard Sandiford
<richard.sandif...@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....@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
> >>
