expmed: TRUNCATE value1 if needed in store_bit_field_using_insv

PR target/113179.

In `store_bit_field_using_insv`, we just use SUBREG if value_mode
>= op_mode, while in some ports, a sign_extend will be needed,
such as MIPS64:
  If either GPR rs or GPR rt does not contain sign-extended 32-bit
  values (bits 63..31 equal), then the result of the operation is
  UNPREDICTABLE.

The problem happens for the code like:
  struct xx {
        int a:4;
        int b:24;
        int c:3;
        int d:1;
  };

  void xx (struct xx *a, long long b) {
        a->d = b;
  }

In the above code, the hard register contains `b`, may be note well
sign-extended.

gcc/
	PR target/113179
	* expmed.c(store_bit_field_using_insv): TRUNCATE value1 if
	needed.

gcc/testsuite
	PR target/113179
	* gcc.target/mips/pr113179.c: New tests.
---
 gcc/expmed.cc                            | 12 +++++++++---
 gcc/testsuite/gcc.target/mips/pr113179.c | 18 ++++++++++++++++++
 2 files changed, 27 insertions(+), 3 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/mips/pr113179.c

YunQiang Su <syq@gcc.gnu.org> writes:
> PR target/113179.
>
> In `store_bit_field_using_insv`, we just use SUBREG if value_mode
>>= op_mode, while in some ports, a sign_extend will be needed,
> such as MIPS64:
>   If either GPR rs or GPR rt does not contain sign-extended 32-bit
>   values (bits 63..31 equal), then the result of the operation is
>   UNPREDICTABLE.
>
> The problem happens for the code like:
>   struct xx {
>         int a:4;
>         int b:24;
>         int c:3;
>         int d:1;
>   };
>
>   void xx (struct xx *a, long long b) {
>         a->d = b;
>   }
>
> In the above code, the hard register contains `b`, may be note well
> sign-extended.
>
> gcc/
> 	PR target/113179
> 	* expmed.c(store_bit_field_using_insv): TRUNCATE value1 if
> 	needed.
>
> gcc/testsuite
> 	PR target/113179
> 	* gcc.target/mips/pr113179.c: New tests.
> ---
>  gcc/expmed.cc                            | 12 +++++++++---
>  gcc/testsuite/gcc.target/mips/pr113179.c | 18 ++++++++++++++++++
>  2 files changed, 27 insertions(+), 3 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/mips/pr113179.c
>
> diff --git a/gcc/expmed.cc b/gcc/expmed.cc
> index 4ec035e4843..6a582593da8 100644
> --- a/gcc/expmed.cc
> +++ b/gcc/expmed.cc
> @@ -704,9 +704,15 @@ store_bit_field_using_insv (const extraction_insn *insv, rtx op0,
>  	    }
>  	  else
>  	    {
> -	      tmp = gen_lowpart_if_possible (op_mode, value1);
> -	      if (! tmp)
> -		tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
> +	      if (targetm.mode_rep_extended (op_mode, value_mode))
> +		tmp = simplify_gen_unary (TRUNCATE, op_mode,
> +					  value1, value_mode);
> +	      else
> +		{
> +		  tmp = gen_lowpart_if_possible (op_mode, value1);
> +		  if (! tmp)
> +		    tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
> +		}
>  	    }
>  	  value1 = tmp;
>  	}

I notice this patch is already applied.  Was it approved?  I didn't
see an approval in my feed or in the archives.

Although it might not make any difference on current targets,
I think the conditional should logically be based on
TRULY_NOOP_TRUNCATION(_MODES_P) rather than targetm.mode_rep_extended.

TRULY_NOOP_TRUNCATION is a correctness question: can I use subregs
to do this truncation?  targetm.mode_rep_extended is instead an
optimisation question: can I assume that a particular extension is free?
Here we're trying to avoid a subreg for correctness, rather than trying
to take advantage of a cheap extension.

So I think the code should be:

	  if (GET_MODE_SIZE (value_mode) < GET_MODE_SIZE (op_mode))
	    {
	      tmp = simplify_subreg (op_mode, value1, value_mode, 0);
	      if (! tmp)
		tmp = simplify_gen_subreg (op_mode,
					   force_reg (value_mode, value1),
					   value_mode, 0);
	    }
	  else if (GET_MODE_SIZE (op_mode) < GET_MODE_SIZE (value_mode)
		   && !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode))
	    tmp = simplify_gen_unary (TRUNCATE, op_mode, value1, value_mode);
	  else
	    {
	      tmp = gen_lowpart_if_possible (op_mode, value1);
	      if (! tmp)
		tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
	    }

(also inclues unnesting of the else).  Could you try changing the code
to do that and push the change if it works?

IMO the patch (in that form) is OK for backports after it has had a
couple of weeks on trunk.

Thanks,
Richard

> diff --git a/gcc/testsuite/gcc.target/mips/pr113179.c b/gcc/testsuite/gcc.target/mips/pr113179.c
> new file mode 100644
> index 00000000000..f32c5a16765
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/pr113179.c
> @@ -0,0 +1,18 @@
> +/* Check if the operand of INS is sign-extended on MIPS64.  */
> +/* { dg-options "-mips64r2 -mabi=64" } */
> +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */
> +
> +struct xx {
> +        int a:1;
> +        int b:24;
> +        int c:6;
> +        int d:1;
> +};
> +
> +long long xx (struct xx *a, long long b) {
> +        a->d = b;
> +        return b+1;
> +}
> +
> +/* { dg-final { scan-assembler "\tsll\t\\\$3,\\\$5,0" } } */
> +/* { dg-final { scan-assembler "\tdaddiu\t\\\$2,\\\$5,1" } } */

On 6/5/24 8:14 AM, Richard Sandiford wrote:
> YunQiang Su <syq@gcc.gnu.org> writes:
>> PR target/113179.
>>
>> In `store_bit_field_using_insv`, we just use SUBREG if value_mode
>>> = op_mode, while in some ports, a sign_extend will be needed,
>> such as MIPS64:
>>    If either GPR rs or GPR rt does not contain sign-extended 32-bit
>>    values (bits 63..31 equal), then the result of the operation is
>>    UNPREDICTABLE.
>>
>> The problem happens for the code like:
>>    struct xx {
>>          int a:4;
>>          int b:24;
>>          int c:3;
>>          int d:1;
>>    };
>>
>>    void xx (struct xx *a, long long b) {
>>          a->d = b;
>>    }
>>
>> In the above code, the hard register contains `b`, may be note well
>> sign-extended.
>>
>> gcc/
>> 	PR target/113179
>> 	* expmed.c(store_bit_field_using_insv): TRUNCATE value1 if
>> 	needed.
>>
>> gcc/testsuite
>> 	PR target/113179
>> 	* gcc.target/mips/pr113179.c: New tests.
>> ---
>>   gcc/expmed.cc                            | 12 +++++++++---
>>   gcc/testsuite/gcc.target/mips/pr113179.c | 18 ++++++++++++++++++
>>   2 files changed, 27 insertions(+), 3 deletions(-)
>>   create mode 100644 gcc/testsuite/gcc.target/mips/pr113179.c
>>
>> diff --git a/gcc/expmed.cc b/gcc/expmed.cc
>> index 4ec035e4843..6a582593da8 100644
>> --- a/gcc/expmed.cc
>> +++ b/gcc/expmed.cc
>> @@ -704,9 +704,15 @@ store_bit_field_using_insv (const extraction_insn *insv, rtx op0,
>>   	    }
>>   	  else
>>   	    {
>> -	      tmp = gen_lowpart_if_possible (op_mode, value1);
>> -	      if (! tmp)
>> -		tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
>> +	      if (targetm.mode_rep_extended (op_mode, value_mode))
>> +		tmp = simplify_gen_unary (TRUNCATE, op_mode,
>> +					  value1, value_mode);
>> +	      else
>> +		{
>> +		  tmp = gen_lowpart_if_possible (op_mode, value1);
>> +		  if (! tmp)
>> +		    tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
>> +		}
>>   	    }
>>   	  value1 = tmp;
>>   	}
> 
> I notice this patch is already applied.  Was it approved?  I didn't
> see an approval in my feed or in the archives.
I don't see an approval and this patch was in my pending-patches box as 
unresolved.

Jeff

Richard Sandiford <richard.sandiford@arm.com> 于2024年6月5日周三 22:14写道：
>
> YunQiang Su <syq@gcc.gnu.org> writes:
> > PR target/113179.
> >
> > In `store_bit_field_using_insv`, we just use SUBREG if value_mode
> >>= op_mode, while in some ports, a sign_extend will be needed,
> > such as MIPS64:
> >   If either GPR rs or GPR rt does not contain sign-extended 32-bit
> >   values (bits 63..31 equal), then the result of the operation is
> >   UNPREDICTABLE.
> >
> > The problem happens for the code like:
> >   struct xx {
> >         int a:4;
> >         int b:24;
> >         int c:3;
> >         int d:1;
> >   };
> >
> >   void xx (struct xx *a, long long b) {
> >         a->d = b;
> >   }
> >
> > In the above code, the hard register contains `b`, may be note well
> > sign-extended.
> >
> > gcc/
> >       PR target/113179
> >       * expmed.c(store_bit_field_using_insv): TRUNCATE value1 if
> >       needed.
> >
> > gcc/testsuite
> >       PR target/113179
> >       * gcc.target/mips/pr113179.c: New tests.
> > ---
> >  gcc/expmed.cc                            | 12 +++++++++---
> >  gcc/testsuite/gcc.target/mips/pr113179.c | 18 ++++++++++++++++++
> >  2 files changed, 27 insertions(+), 3 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.target/mips/pr113179.c
> >
> > diff --git a/gcc/expmed.cc b/gcc/expmed.cc
> > index 4ec035e4843..6a582593da8 100644
> > --- a/gcc/expmed.cc
> > +++ b/gcc/expmed.cc
> > @@ -704,9 +704,15 @@ store_bit_field_using_insv (const extraction_insn *insv, rtx op0,
> >           }
> >         else
> >           {
> > -           tmp = gen_lowpart_if_possible (op_mode, value1);
> > -           if (! tmp)
> > -             tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
> > +           if (targetm.mode_rep_extended (op_mode, value_mode))
> > +             tmp = simplify_gen_unary (TRUNCATE, op_mode,
> > +                                       value1, value_mode);
> > +           else
> > +             {
> > +               tmp = gen_lowpart_if_possible (op_mode, value1);
> > +               if (! tmp)
> > +                 tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
> > +             }
> >           }
> >         value1 = tmp;
> >       }
>
> I notice this patch is already applied.  Was it approved?  I didn't
> see an approval in my feed or in the archives.
>

Sorry. I was supposed that it only effects MIPS targets since only MIPS defines
  targetm.mode_rep_extended

> Although it might not make any difference on current targets,
> I think the conditional should logically be based on
> TRULY_NOOP_TRUNCATION(_MODES_P) rather than targetm.mode_rep_extended.
>
> TRULY_NOOP_TRUNCATION is a correctness question: can I use subregs
> to do this truncation?  targetm.mode_rep_extended is instead an
> optimisation question: can I assume that a particular extension is free?
> Here we're trying to avoid a subreg for correctness, rather than trying
> to take advantage of a cheap extension.
>
> So I think the code should be:
>
>           if (GET_MODE_SIZE (value_mode) < GET_MODE_SIZE (op_mode))
>             {
>               tmp = simplify_subreg (op_mode, value1, value_mode, 0);
>               if (! tmp)
>                 tmp = simplify_gen_subreg (op_mode,
>                                            force_reg (value_mode, value1),
>                                            value_mode, 0);
>             }
>           else if (GET_MODE_SIZE (op_mode) < GET_MODE_SIZE (value_mode)
>                    && !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode))

In fact I don't think so. For other targets besides MIPS, it is safe even
    !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode)
as INS instruction may use the low part of a register safely.

It is only not true on MIPS ISA documents as
     If either GPR rs or GPR rt does not contain sign-extended 32-bit
     values (bits 63..31 equal), then the result of the operation is
     UNPREDICTABLE.

It is very annoying. I haven't noticed a similar problem on any other
ISA documents.
In fact I don't know any real MIPS hardware that is "UNPREDICTABLE" in
this case.

>             tmp = simplify_gen_unary (TRUNCATE, op_mode, value1, value_mode);
>           else
>             {
>               tmp = gen_lowpart_if_possible (op_mode, value1);
>               if (! tmp)
>                 tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
>             }
>
> (also inclues unnesting of the else).  Could you try changing the code
> to do that and push the change if it works?
>
> IMO the patch (in that form) is OK for backports after it has had a
> couple of weeks on trunk.
>
> Thanks,
> Richard
>
> > diff --git a/gcc/testsuite/gcc.target/mips/pr113179.c b/gcc/testsuite/gcc.target/mips/pr113179.c
> > new file mode 100644
> > index 00000000000..f32c5a16765
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/mips/pr113179.c
> > @@ -0,0 +1,18 @@
> > +/* Check if the operand of INS is sign-extended on MIPS64.  */
> > +/* { dg-options "-mips64r2 -mabi=64" } */
> > +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */
> > +
> > +struct xx {
> > +        int a:1;
> > +        int b:24;
> > +        int c:6;
> > +        int d:1;
> > +};
> > +
> > +long long xx (struct xx *a, long long b) {
> > +        a->d = b;
> > +        return b+1;
> > +}
> > +
> > +/* { dg-final { scan-assembler "\tsll\t\\\$3,\\\$5,0" } } */
> > +/* { dg-final { scan-assembler "\tdaddiu\t\\\$2,\\\$5,1" } } */

YunQiang Su <syq@gcc.gnu.org> writes:
> Richard Sandiford <richard.sandiford@arm.com> 于2024年6月5日周三 22:14写道：
>>
>> YunQiang Su <syq@gcc.gnu.org> writes:
>> > PR target/113179.
>> >
>> > In `store_bit_field_using_insv`, we just use SUBREG if value_mode
>> >>= op_mode, while in some ports, a sign_extend will be needed,
>> > such as MIPS64:
>> >   If either GPR rs or GPR rt does not contain sign-extended 32-bit
>> >   values (bits 63..31 equal), then the result of the operation is
>> >   UNPREDICTABLE.
>> >
>> > The problem happens for the code like:
>> >   struct xx {
>> >         int a:4;
>> >         int b:24;
>> >         int c:3;
>> >         int d:1;
>> >   };
>> >
>> >   void xx (struct xx *a, long long b) {
>> >         a->d = b;
>> >   }
>> >
>> > In the above code, the hard register contains `b`, may be note well
>> > sign-extended.
>> >
>> > gcc/
>> >       PR target/113179
>> >       * expmed.c(store_bit_field_using_insv): TRUNCATE value1 if
>> >       needed.
>> >
>> > gcc/testsuite
>> >       PR target/113179
>> >       * gcc.target/mips/pr113179.c: New tests.
>> > ---
>> >  gcc/expmed.cc                            | 12 +++++++++---
>> >  gcc/testsuite/gcc.target/mips/pr113179.c | 18 ++++++++++++++++++
>> >  2 files changed, 27 insertions(+), 3 deletions(-)
>> >  create mode 100644 gcc/testsuite/gcc.target/mips/pr113179.c
>> >
>> > diff --git a/gcc/expmed.cc b/gcc/expmed.cc
>> > index 4ec035e4843..6a582593da8 100644
>> > --- a/gcc/expmed.cc
>> > +++ b/gcc/expmed.cc
>> > @@ -704,9 +704,15 @@ store_bit_field_using_insv (const extraction_insn *insv, rtx op0,
>> >           }
>> >         else
>> >           {
>> > -           tmp = gen_lowpart_if_possible (op_mode, value1);
>> > -           if (! tmp)
>> > -             tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
>> > +           if (targetm.mode_rep_extended (op_mode, value_mode))
>> > +             tmp = simplify_gen_unary (TRUNCATE, op_mode,
>> > +                                       value1, value_mode);
>> > +           else
>> > +             {
>> > +               tmp = gen_lowpart_if_possible (op_mode, value1);
>> > +               if (! tmp)
>> > +                 tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
>> > +             }
>> >           }
>> >         value1 = tmp;
>> >       }
>>
>> I notice this patch is already applied.  Was it approved?  I didn't
>> see an approval in my feed or in the archives.
>>
>
> Sorry. I was supposed that it only effects MIPS targets since only MIPS defines
>   targetm.mode_rep_extended
>
>> Although it might not make any difference on current targets,
>> I think the conditional should logically be based on
>> TRULY_NOOP_TRUNCATION(_MODES_P) rather than targetm.mode_rep_extended.
>>
>> TRULY_NOOP_TRUNCATION is a correctness question: can I use subregs
>> to do this truncation?  targetm.mode_rep_extended is instead an
>> optimisation question: can I assume that a particular extension is free?
>> Here we're trying to avoid a subreg for correctness, rather than trying
>> to take advantage of a cheap extension.
>>
>> So I think the code should be:
>>
>>           if (GET_MODE_SIZE (value_mode) < GET_MODE_SIZE (op_mode))
>>             {
>>               tmp = simplify_subreg (op_mode, value1, value_mode, 0);
>>               if (! tmp)
>>                 tmp = simplify_gen_subreg (op_mode,
>>                                            force_reg (value_mode, value1),
>>                                            value_mode, 0);
>>             }
>>           else if (GET_MODE_SIZE (op_mode) < GET_MODE_SIZE (value_mode)
>>                    && !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode))
>
> In fact I don't think so. For other targets besides MIPS, it is safe even
>     !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode)
> as INS instruction may use the low part of a register safely.

Not sure what you mean.  The change above is no-op for targets other
than MIPS and SH5 (now removed).  But I think it's the correct way of
representing the restriction on MIPS and SH5.

>
> It is only not true on MIPS ISA documents as
>      If either GPR rs or GPR rt does not contain sign-extended 32-bit
>      values (bits 63..31 equal), then the result of the operation is
>      UNPREDICTABLE.

Right.  The reason that TRULY_NOOP_TRUNCATION_MODES_P (SImode, DImode)
is false for MIPS isn't that a subreg is impossible on MIPS targets.
It's that the MIPS port needs to ensure that SImode values are stored
in sign-extended form in order to avoid the problem above.  So a
truncation needs to be a sign-extension.

> It is very annoying. I haven't noticed a similar problem on any other
> ISA documents.
> In fact I don't know any real MIPS hardware that is "UNPREDICTABLE" in
> this case.

I think the Broadcom SB-1 took advantage of the freedom.

Thanks,
Richard

On 6/5/24 8:57 AM, YunQiang Su wrote:
> Richard Sandiford <richard.sandiford@arm.com> 于2024年6月5日周三 22:14写道：
>>
>> YunQiang Su <syq@gcc.gnu.org> writes:
>>> PR target/113179.
>>>
>>> In `store_bit_field_using_insv`, we just use SUBREG if value_mode
>>>> = op_mode, while in some ports, a sign_extend will be needed,
>>> such as MIPS64:
>>>    If either GPR rs or GPR rt does not contain sign-extended 32-bit
>>>    values (bits 63..31 equal), then the result of the operation is
>>>    UNPREDICTABLE.
>>>
>>> The problem happens for the code like:
>>>    struct xx {
>>>          int a:4;
>>>          int b:24;
>>>          int c:3;
>>>          int d:1;
>>>    };
>>>
>>>    void xx (struct xx *a, long long b) {
>>>          a->d = b;
>>>    }
>>>
>>> In the above code, the hard register contains `b`, may be note well
>>> sign-extended.
>>>
>>> gcc/
>>>        PR target/113179
>>>        * expmed.c(store_bit_field_using_insv): TRUNCATE value1 if
>>>        needed.
>>>
>>> gcc/testsuite
>>>        PR target/113179
>>>        * gcc.target/mips/pr113179.c: New tests.
>>> ---
>>>   gcc/expmed.cc                            | 12 +++++++++---
>>>   gcc/testsuite/gcc.target/mips/pr113179.c | 18 ++++++++++++++++++
>>>   2 files changed, 27 insertions(+), 3 deletions(-)
>>>   create mode 100644 gcc/testsuite/gcc.target/mips/pr113179.c
>>>
>>> diff --git a/gcc/expmed.cc b/gcc/expmed.cc
>>> index 4ec035e4843..6a582593da8 100644
>>> --- a/gcc/expmed.cc
>>> +++ b/gcc/expmed.cc
>>> @@ -704,9 +704,15 @@ store_bit_field_using_insv (const extraction_insn *insv, rtx op0,
>>>            }
>>>          else
>>>            {
>>> -           tmp = gen_lowpart_if_possible (op_mode, value1);
>>> -           if (! tmp)
>>> -             tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
>>> +           if (targetm.mode_rep_extended (op_mode, value_mode))
>>> +             tmp = simplify_gen_unary (TRUNCATE, op_mode,
>>> +                                       value1, value_mode);
>>> +           else
>>> +             {
>>> +               tmp = gen_lowpart_if_possible (op_mode, value1);
>>> +               if (! tmp)
>>> +                 tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
>>> +             }
>>>            }
>>>          value1 = tmp;
>>>        }
>>
>> I notice this patch is already applied.  Was it approved?  I didn't
>> see an approval in my feed or in the archives.
>>
> 
> Sorry. I was supposed that it only effects MIPS targets since only MIPS defines
>    targetm.mode_rep_extended
Just a note for the future, even if something is guarded by a target 
hook that's only defined by a single target we'd want to see an ACK if 
the code is in a target independent file.

Jeff

Richard Sandiford <richard.sandiford@arm.com> 于2024年6月5日周三 23:20写道：
>
> YunQiang Su <syq@gcc.gnu.org> writes:
> > Richard Sandiford <richard.sandiford@arm.com> 于2024年6月5日周三 22:14写道：
> >>
> >> YunQiang Su <syq@gcc.gnu.org> writes:
> >> > PR target/113179.
> >> >
> >> > In `store_bit_field_using_insv`, we just use SUBREG if value_mode
> >> >>= op_mode, while in some ports, a sign_extend will be needed,
> >> > such as MIPS64:
> >> >   If either GPR rs or GPR rt does not contain sign-extended 32-bit
> >> >   values (bits 63..31 equal), then the result of the operation is
> >> >   UNPREDICTABLE.
> >> >
> >> > The problem happens for the code like:
> >> >   struct xx {
> >> >         int a:4;
> >> >         int b:24;
> >> >         int c:3;
> >> >         int d:1;
> >> >   };
> >> >
> >> >   void xx (struct xx *a, long long b) {
> >> >         a->d = b;
> >> >   }
> >> >
> >> > In the above code, the hard register contains `b`, may be note well
> >> > sign-extended.
> >> >
> >> > gcc/
> >> >       PR target/113179
> >> >       * expmed.c(store_bit_field_using_insv): TRUNCATE value1 if
> >> >       needed.
> >> >
> >> > gcc/testsuite
> >> >       PR target/113179
> >> >       * gcc.target/mips/pr113179.c: New tests.
> >> > ---
> >> >  gcc/expmed.cc                            | 12 +++++++++---
> >> >  gcc/testsuite/gcc.target/mips/pr113179.c | 18 ++++++++++++++++++
> >> >  2 files changed, 27 insertions(+), 3 deletions(-)
> >> >  create mode 100644 gcc/testsuite/gcc.target/mips/pr113179.c
> >> >
> >> > diff --git a/gcc/expmed.cc b/gcc/expmed.cc
> >> > index 4ec035e4843..6a582593da8 100644
> >> > --- a/gcc/expmed.cc
> >> > +++ b/gcc/expmed.cc
> >> > @@ -704,9 +704,15 @@ store_bit_field_using_insv (const extraction_insn *insv, rtx op0,
> >> >           }
> >> >         else
> >> >           {
> >> > -           tmp = gen_lowpart_if_possible (op_mode, value1);
> >> > -           if (! tmp)
> >> > -             tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
> >> > +           if (targetm.mode_rep_extended (op_mode, value_mode))
> >> > +             tmp = simplify_gen_unary (TRUNCATE, op_mode,
> >> > +                                       value1, value_mode);
> >> > +           else
> >> > +             {
> >> > +               tmp = gen_lowpart_if_possible (op_mode, value1);
> >> > +               if (! tmp)
> >> > +                 tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
> >> > +             }
> >> >           }
> >> >         value1 = tmp;
> >> >       }
> >>
> >> I notice this patch is already applied.  Was it approved?  I didn't
> >> see an approval in my feed or in the archives.
> >>
> >
> > Sorry. I was supposed that it only effects MIPS targets since only MIPS defines
> >   targetm.mode_rep_extended
> >
> >> Although it might not make any difference on current targets,
> >> I think the conditional should logically be based on
> >> TRULY_NOOP_TRUNCATION(_MODES_P) rather than targetm.mode_rep_extended.
> >>
> >> TRULY_NOOP_TRUNCATION is a correctness question: can I use subregs
> >> to do this truncation?  targetm.mode_rep_extended is instead an
> >> optimisation question: can I assume that a particular extension is free?
> >> Here we're trying to avoid a subreg for correctness, rather than trying
> >> to take advantage of a cheap extension.
> >>
> >> So I think the code should be:
> >>
> >>           if (GET_MODE_SIZE (value_mode) < GET_MODE_SIZE (op_mode))
> >>             {
> >>               tmp = simplify_subreg (op_mode, value1, value_mode, 0);
> >>               if (! tmp)
> >>                 tmp = simplify_gen_subreg (op_mode,
> >>                                            force_reg (value_mode, value1),
> >>                                            value_mode, 0);
> >>             }
> >>           else if (GET_MODE_SIZE (op_mode) < GET_MODE_SIZE (value_mode)
> >>                    && !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode))
> >
> > In fact I don't think so. For other targets besides MIPS, it is safe even
> >     !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode)
> > as INS instruction may use the low part of a register safely.
>
> Not sure what you mean.  The change above is no-op for targets other
> than MIPS and SH5 (now removed).  But I think it's the correct way of
> representing the restriction on MIPS and SH5.
>
> >
> > It is only not true on MIPS ISA documents as
> >      If either GPR rs or GPR rt does not contain sign-extended 32-bit
> >      values (bits 63..31 equal), then the result of the operation is
> >      UNPREDICTABLE.
>
> Right.  The reason that TRULY_NOOP_TRUNCATION_MODES_P (SImode, DImode)
> is false for MIPS isn't that a subreg is impossible on MIPS targets.
> It's that the MIPS port needs to ensure that SImode values are stored
> in sign-extended form in order to avoid the problem above.  So a
> truncation needs to be a sign-extension.
>

Thanks for your explanation. NVPTX defines TRULY_NOOP_TRUNCATION_MODES_P
to always false.

and

bool
gcn_truly_noop_truncation (poly_uint64 outprec, poly_uint64 inprec)
{
  return ((inprec <= 32) && (outprec <= inprec));
}

I am worrying that we may break nvptx/amdgcn as I have little
knowledge about them.


> > It is very annoying. I haven't noticed a similar problem on any other
> > ISA documents.
> > In fact I don't know any real MIPS hardware that is "UNPREDICTABLE" in
> > this case.
>
> I think the Broadcom SB-1 took advantage of the freedom.
>
> Thanks,
> Richard

YunQiang Su <syq@gcc.gnu.org> writes:
> Richard Sandiford <richard.sandiford@arm.com> 于2024年6月5日周三 23:20写道：
>>
>> YunQiang Su <syq@gcc.gnu.org> writes:
>> > Richard Sandiford <richard.sandiford@arm.com> 于2024年6月5日周三 22:14写道：
>> >>
>> >> YunQiang Su <syq@gcc.gnu.org> writes:
>> >> > PR target/113179.
>> >> >
>> >> > In `store_bit_field_using_insv`, we just use SUBREG if value_mode
>> >> >>= op_mode, while in some ports, a sign_extend will be needed,
>> >> > such as MIPS64:
>> >> >   If either GPR rs or GPR rt does not contain sign-extended 32-bit
>> >> >   values (bits 63..31 equal), then the result of the operation is
>> >> >   UNPREDICTABLE.
>> >> >
>> >> > The problem happens for the code like:
>> >> >   struct xx {
>> >> >         int a:4;
>> >> >         int b:24;
>> >> >         int c:3;
>> >> >         int d:1;
>> >> >   };
>> >> >
>> >> >   void xx (struct xx *a, long long b) {
>> >> >         a->d = b;
>> >> >   }
>> >> >
>> >> > In the above code, the hard register contains `b`, may be note well
>> >> > sign-extended.
>> >> >
>> >> > gcc/
>> >> >       PR target/113179
>> >> >       * expmed.c(store_bit_field_using_insv): TRUNCATE value1 if
>> >> >       needed.
>> >> >
>> >> > gcc/testsuite
>> >> >       PR target/113179
>> >> >       * gcc.target/mips/pr113179.c: New tests.
>> >> > ---
>> >> >  gcc/expmed.cc                            | 12 +++++++++---
>> >> >  gcc/testsuite/gcc.target/mips/pr113179.c | 18 ++++++++++++++++++
>> >> >  2 files changed, 27 insertions(+), 3 deletions(-)
>> >> >  create mode 100644 gcc/testsuite/gcc.target/mips/pr113179.c
>> >> >
>> >> > diff --git a/gcc/expmed.cc b/gcc/expmed.cc
>> >> > index 4ec035e4843..6a582593da8 100644
>> >> > --- a/gcc/expmed.cc
>> >> > +++ b/gcc/expmed.cc
>> >> > @@ -704,9 +704,15 @@ store_bit_field_using_insv (const extraction_insn *insv, rtx op0,
>> >> >           }
>> >> >         else
>> >> >           {
>> >> > -           tmp = gen_lowpart_if_possible (op_mode, value1);
>> >> > -           if (! tmp)
>> >> > -             tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
>> >> > +           if (targetm.mode_rep_extended (op_mode, value_mode))
>> >> > +             tmp = simplify_gen_unary (TRUNCATE, op_mode,
>> >> > +                                       value1, value_mode);
>> >> > +           else
>> >> > +             {
>> >> > +               tmp = gen_lowpart_if_possible (op_mode, value1);
>> >> > +               if (! tmp)
>> >> > +                 tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
>> >> > +             }
>> >> >           }
>> >> >         value1 = tmp;
>> >> >       }
>> >>
>> >> I notice this patch is already applied.  Was it approved?  I didn't
>> >> see an approval in my feed or in the archives.
>> >>
>> >
>> > Sorry. I was supposed that it only effects MIPS targets since only MIPS defines
>> >   targetm.mode_rep_extended
>> >
>> >> Although it might not make any difference on current targets,
>> >> I think the conditional should logically be based on
>> >> TRULY_NOOP_TRUNCATION(_MODES_P) rather than targetm.mode_rep_extended.
>> >>
>> >> TRULY_NOOP_TRUNCATION is a correctness question: can I use subregs
>> >> to do this truncation?  targetm.mode_rep_extended is instead an
>> >> optimisation question: can I assume that a particular extension is free?
>> >> Here we're trying to avoid a subreg for correctness, rather than trying
>> >> to take advantage of a cheap extension.
>> >>
>> >> So I think the code should be:
>> >>
>> >>           if (GET_MODE_SIZE (value_mode) < GET_MODE_SIZE (op_mode))
>> >>             {
>> >>               tmp = simplify_subreg (op_mode, value1, value_mode, 0);
>> >>               if (! tmp)
>> >>                 tmp = simplify_gen_subreg (op_mode,
>> >>                                            force_reg (value_mode, value1),
>> >>                                            value_mode, 0);
>> >>             }
>> >>           else if (GET_MODE_SIZE (op_mode) < GET_MODE_SIZE (value_mode)
>> >>                    && !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode))
>> >
>> > In fact I don't think so. For other targets besides MIPS, it is safe even
>> >     !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode)
>> > as INS instruction may use the low part of a register safely.
>>
>> Not sure what you mean.  The change above is no-op for targets other
>> than MIPS and SH5 (now removed).  But I think it's the correct way of
>> representing the restriction on MIPS and SH5.
>>
>> >
>> > It is only not true on MIPS ISA documents as
>> >      If either GPR rs or GPR rt does not contain sign-extended 32-bit
>> >      values (bits 63..31 equal), then the result of the operation is
>> >      UNPREDICTABLE.
>>
>> Right.  The reason that TRULY_NOOP_TRUNCATION_MODES_P (SImode, DImode)
>> is false for MIPS isn't that a subreg is impossible on MIPS targets.
>> It's that the MIPS port needs to ensure that SImode values are stored
>> in sign-extended form in order to avoid the problem above.  So a
>> truncation needs to be a sign-extension.
>>
>
> Thanks for your explanation. NVPTX defines TRULY_NOOP_TRUNCATION_MODES_P
> to always false.

Yeah.  AIUI, the change above behaves correcty there.  It maintains
the type correctness of the output.

> and
>
> bool
> gcn_truly_noop_truncation (poly_uint64 outprec, poly_uint64 inprec)
> {
>   return ((inprec <= 32) && (outprec <= inprec));
> }

Not sure why GCN requires this TBH, but...

> I am worrying that we may break nvptx/amdgcn as I have little
> knowledge about them.

...I think the targets are asking for the same behaviour as MIPS.
The reason for the patch is that the code really is doing a
truncation from value_mode to op_mode.  If the target says that
that isn't a no-op, we have to use the target's truncation pattern
instead of a subreg.

Thanks,
Richard

>
>
>> > It is very annoying. I haven't noticed a similar problem on any other
>> > ISA documents.
>> > In fact I don't know any real MIPS hardware that is "UNPREDICTABLE" in
>> > this case.
>>
>> I think the Broadcom SB-1 took advantage of the freedom.
>>
>> Thanks,
>> Richard