diff mbox series

[RFC] Store_bit_field_1: Use mode of SUBREG instead of REG

Message ID 20230712031935.3908564-1-yunqiang.su@cipunited.com
State Accepted
Headers
Series [RFC] Store_bit_field_1: Use mode of SUBREG instead of REG |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

YunQiang Su July 12, 2023, 3:19 a.m. UTC 
  PR #104914

When work with
  int val;
  ((unsigned char*)&val)[0] = *buf;
The RTX mode is obtained from REG instead of SUBREG,
which make D<INS> is used instead of <INS>.
Thus something wrong happens on sign-extend default architectures,
like MIPS64.

gcc/ChangeLog:
	PR: 104914.
	* expmed.cc(store_bit_field_1): Get mode from original
	str_rtx instead of op0.
---
 gcc/expmed.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Richard Biener July 12, 2023, 7:44 a.m. UTC | #1 
On Wed, Jul 12, 2023 at 5:20 AM YunQiang Su <yunqiang.su@cipunited.com> wrote:
>
> PR #104914
>
> When work with
>   int val;
>   ((unsigned char*)&val)[0] = *buf;
> The RTX mode is obtained from REG instead of SUBREG,
> which make D<INS> is used instead of <INS>.
> Thus something wrong happens on sign-extend default architectures,
> like MIPS64.
>
> gcc/ChangeLog:
>         PR: 104914.
>         * expmed.cc(store_bit_field_1): Get mode from original
>         str_rtx instead of op0.
> ---
>  gcc/expmed.cc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/gcc/expmed.cc b/gcc/expmed.cc
> index fbd4ce2d42f..37f90912122 100644
> --- a/gcc/expmed.cc
> +++ b/gcc/expmed.cc
> @@ -849,7 +849,7 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize, poly_uint64 bitnum,
>       if we aren't.  This must come after the entire register case above,
>       since that case is valid for any mode.  The following cases are only
>       valid for integral modes.  */
> -  opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (op0));
> +  opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (str_rtx));

I don't think this is correct - op0_mode is used to store into op0, and we are
just requiring that it is an integer mode and equal to the original
mode.  I suppose
your patch makes us go to the fallback code instead, but it's surely
for the wrong
reason.  I also wonder why we don't just check GET_MODE_CLASS
(GET_MODE (op0)) == MODE_CLASS_INT ...

>    scalar_int_mode imode;
>    if (!op0_mode.exists (&imode) || imode != GET_MODE (op0))
>      {
> --
> 2.30.2
>
YunQiang Su July 12, 2023, 11:22 a.m. UTC | #2 
> 2023年7月12日 15:44，Richard Biener <richard.guenther@gmail.com> 写道：
> 
> On Wed, Jul 12, 2023 at 5:20 AM YunQiang Su <yunqiang.su@cipunited.com> wrote:
>> 
>> PR #104914
>> 
>> When work with
>>  int val;
>>  ((unsigned char*)&val)[0] = *buf;
>> The RTX mode is obtained from REG instead of SUBREG,
>> which make D<INS> is used instead of <INS>.
>> Thus something wrong happens on sign-extend default architectures,
>> like MIPS64.
>> 
>> gcc/ChangeLog:
>>        PR: 104914.
>>        * expmed.cc(store_bit_field_1): Get mode from original
>>        str_rtx instead of op0.
>> ---
>> gcc/expmed.cc | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/gcc/expmed.cc b/gcc/expmed.cc
>> index fbd4ce2d42f..37f90912122 100644
>> --- a/gcc/expmed.cc
>> +++ b/gcc/expmed.cc
>> @@ -849,7 +849,7 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize, poly_uint64 bitnum,
>>      if we aren't.  This must come after the entire register case above,
>>      since that case is valid for any mode.  The following cases are only
>>      valid for integral modes.  */
>> -  opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (op0));
>> +  opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (str_rtx));
> 
> I don't think this is correct - op0_mode is used to store into op0, and we are
> just requiring that it is an integer mode and equal to the original
> mode.  I suppose
> your patch makes us go to the fallback code instead, but it's surely
> for the wrong

diff --git a/gcc/expmed.cc b/gcc/expmed.cc
index fbd4ce2d42f..feee8c82f59 100644
--- a/gcc/expmed.cc
+++ b/gcc/expmed.cc
@@ -850,6 +861,7 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize, poly_uint64 bitnum,
      since that case is valid for any mode.  The following cases are only
      valid for integral modes.  */
   opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (op0));
+  opt_scalar_int_mode str_mode = int_mode_for_mode (GET_MODE (str_rtx));
   scalar_int_mode imode;
   if (!op0_mode.exists (&imode) || imode != GET_MODE (op0))
     {
@@ -881,8 +893,14 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize, poly_uint64 bitnum,
 	op0 = gen_lowpart (op0_mode.require (), op0);
     }
 
-  return store_integral_bit_field (op0, op0_mode, ibitsize, ibitnum,
-				   bitregion_start, bitregion_end,
+  bool use_str_mode = false;
+  if (GET_MODE_CLASS(GET_MODE (str_rtx)) == MODE_INT
+      && GET_MODE_CLASS(GET_MODE (op0)) == MODE_INT
+      && known_gt (GET_MODE_SIZE(GET_MODE(op0)), GET_MODE_SIZE(GET_MODE(str_rtx))))
+	use_str_mode = true;
+  return store_integral_bit_field (op0,
+				   use_str_mode ? str_mode : op0_mode,
+				   ibitsize, ibitnum, bitregion_start, bitregion_end,
 				   fieldmode, value, reverse, fallback_p);
 }

> reason.  I also wonder why we don't just check GET_MODE_CLASS
> (GET_MODE (op0)) == MODE_CLASS_INT ...
> 

In fact I have no idea. Maybe there are some other tricky cases.

>>   scalar_int_mode imode;
>>   if (!op0_mode.exists (&imode) || imode != GET_MODE (op0))
>>     {
>> --
>> 2.30.2
diff mbox series

Patch

diff --git a/gcc/expmed.cc b/gcc/expmed.cc
index fbd4ce2d42f..37f90912122 100644
--- a/gcc/expmed.cc
+++ b/gcc/expmed.cc
@@ -849,7 +849,7 @@  store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize, poly_uint64 bitnum,
      if we aren't.  This must come after the entire register case above,
      since that case is valid for any mode.  The following cases are only
      valid for integral modes.  */
-  opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (op0));
+  opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (str_rtx));
   scalar_int_mode imode;
   if (!op0_mode.exists (&imode) || imode != GET_MODE (op0))
     {