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

Richard Sandiford richard.sandiford@arm.com
Fri Aug 6 09:05:43 GMT 2021


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.

>> +      && 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
>>


More information about the Gcc-patches mailing list