diff mbox series

[v3,1/3] rtl: directly handle MEM in gen_highpart [PR102125]

Message ID 20210910144841.3139174-2-rearnsha@arm.com
State New
Headers show
Series lower more cases of memcpy [PR102125] | expand

Commit Message

Richard Earnshaw Sept. 10, 2021, 2:48 p.m. UTC 
gen_lowpart_general handles forming a lowpart of a MEM by using
adjust_address to rework and validate a new version of the MEM.
Do the same for gen_highpart rather than calling simplify_gen_subreg
for this case.

gcc/ChangeLog:

	PR target/102125
	* emit-rtl.c (gen_highpart): Use adjust_address to handle
	MEM rather than calling simplify_gen_subreg.
---
 gcc/emit-rtl.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

Comments

Richard Biener Sept. 13, 2021, 9:38 a.m. UTC | #1 
On Fri, Sep 10, 2021 at 4:48 PM Richard Earnshaw <rearnsha@arm.com> wrote:
>
>
> gen_lowpart_general handles forming a lowpart of a MEM by using
> adjust_address to rework and validate a new version of the MEM.
> Do the same for gen_highpart rather than calling simplify_gen_subreg
> for this case.

OK from my side.

Thanks,
Richard.

> gcc/ChangeLog:
>
>         PR target/102125
>         * emit-rtl.c (gen_highpart): Use adjust_address to handle
>         MEM rather than calling simplify_gen_subreg.
> ---
>  gcc/emit-rtl.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
Richard Sandiford Sept. 13, 2021, 9:38 a.m. UTC | #2 
Richard Earnshaw via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> gen_lowpart_general handles forming a lowpart of a MEM by using
> adjust_address to rework and validate a new version of the MEM.
> Do the same for gen_highpart rather than calling simplify_gen_subreg
> for this case.

Looks OK, but what went wrong with the existing code?  Did
simplify_gen_subreg refuse to handle a MEM that you wanted
it to handle, or did the validize_mem go wrong for some reason?

Thanks,
Richard

> gcc/ChangeLog:
>
> 	PR target/102125
> 	* emit-rtl.c (gen_highpart): Use adjust_address to handle
> 	MEM rather than calling simplify_gen_subreg.
> ---
>  gcc/emit-rtl.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index 77ea8948ee8..0ba110879aa 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -1585,19 +1585,22 @@ gen_highpart (machine_mode mode, rtx x)
>    gcc_assert (known_le (msize, (unsigned int) UNITS_PER_WORD)
>  	      || known_eq (msize, GET_MODE_UNIT_SIZE (GET_MODE (x))));
>  
> -  result = simplify_gen_subreg (mode, x, GET_MODE (x),
> -				subreg_highpart_offset (mode, GET_MODE (x)));
> -  gcc_assert (result);
> -
> -  /* simplify_gen_subreg is not guaranteed to return a valid operand for
> -     the target if we have a MEM.  gen_highpart must return a valid operand,
> -     emitting code if necessary to do so.  */
> -  if (MEM_P (result))
> +  /* gen_lowpart_common handles a lot of special cases due to needing to handle
> +     paradoxical subregs; it only calls simplify_gen_subreg when certain that
> +     it will produce something meaningful.  The only case we need to handle
> +     specially here is MEM.  */
> +  if (MEM_P (x))
>      {
> -      result = validize_mem (result);
> -      gcc_assert (result);
> +      poly_int64 offset = subreg_highpart_offset (mode, GET_MODE (x));
> +      return adjust_address (x, mode, offset);
>      }
>  
> +  result = simplify_gen_subreg (mode, x, GET_MODE (x),
> +				subreg_highpart_offset (mode, GET_MODE (x)));
> +  /* Since we handle MEM directly above, we should never get a MEM back
> +     from simplify_gen_subreg.  */
> +  gcc_assert (result && !MEM_P (result));
> +
>    return result;
>  }
>
Richard Earnshaw Sept. 13, 2021, 9:51 a.m. UTC | #3 
On 13/09/2021 10:38, Richard Sandiford via Gcc-patches wrote:
> Richard Earnshaw via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> gen_lowpart_general handles forming a lowpart of a MEM by using
>> adjust_address to rework and validate a new version of the MEM.
>> Do the same for gen_highpart rather than calling simplify_gen_subreg
>> for this case.
> 
> Looks OK, but what went wrong with the existing code?  Did
> simplify_gen_subreg refuse to handle a MEM that you wanted
> it to handle, or did the validize_mem go wrong for some reason?

It refused to handle it and simply returned (subreg (mem)) - see the 
discussion on version 1 of the patch series.

R.

> 
> Thanks,
> Richard
> 
>> gcc/ChangeLog:
>>
>> 	PR target/102125
>> 	* emit-rtl.c (gen_highpart): Use adjust_address to handle
>> 	MEM rather than calling simplify_gen_subreg.
>> ---
>>   gcc/emit-rtl.c | 23 +++++++++++++----------
>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
>> index 77ea8948ee8..0ba110879aa 100644
>> --- a/gcc/emit-rtl.c
>> +++ b/gcc/emit-rtl.c
>> @@ -1585,19 +1585,22 @@ gen_highpart (machine_mode mode, rtx x)
>>     gcc_assert (known_le (msize, (unsigned int) UNITS_PER_WORD)
>>   	      || known_eq (msize, GET_MODE_UNIT_SIZE (GET_MODE (x))));
>>   
>> -  result = simplify_gen_subreg (mode, x, GET_MODE (x),
>> -				subreg_highpart_offset (mode, GET_MODE (x)));
>> -  gcc_assert (result);
>> -
>> -  /* simplify_gen_subreg is not guaranteed to return a valid operand for
>> -     the target if we have a MEM.  gen_highpart must return a valid operand,
>> -     emitting code if necessary to do so.  */
>> -  if (MEM_P (result))
>> +  /* gen_lowpart_common handles a lot of special cases due to needing to handle
>> +     paradoxical subregs; it only calls simplify_gen_subreg when certain that
>> +     it will produce something meaningful.  The only case we need to handle
>> +     specially here is MEM.  */
>> +  if (MEM_P (x))
>>       {
>> -      result = validize_mem (result);
>> -      gcc_assert (result);
>> +      poly_int64 offset = subreg_highpart_offset (mode, GET_MODE (x));
>> +      return adjust_address (x, mode, offset);
>>       }
>>   
>> +  result = simplify_gen_subreg (mode, x, GET_MODE (x),
>> +				subreg_highpart_offset (mode, GET_MODE (x)));
>> +  /* Since we handle MEM directly above, we should never get a MEM back
>> +     from simplify_gen_subreg.  */
>> +  gcc_assert (result && !MEM_P (result));
>> +
>>     return result;
>>   }
>>
Richard Sandiford Sept. 13, 2021, 9:54 a.m. UTC | #4 
Richard Earnshaw <Richard.Earnshaw@foss.arm.com> writes:
> On 13/09/2021 10:38, Richard Sandiford via Gcc-patches wrote:
>> Richard Earnshaw via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>>> gen_lowpart_general handles forming a lowpart of a MEM by using
>>> adjust_address to rework and validate a new version of the MEM.
>>> Do the same for gen_highpart rather than calling simplify_gen_subreg
>>> for this case.
>> 
>> Looks OK, but what went wrong with the existing code?  Did
>> simplify_gen_subreg refuse to handle a MEM that you wanted
>> it to handle, or did the validize_mem go wrong for some reason?
>
> It refused to handle it and simply returned (subreg (mem)) - see the 
> discussion on version 1 of the patch series.

OK, that's good then.  The patch is OK from my POV too FWIW.

Richard

>>> gcc/ChangeLog:
>>>
>>> 	PR target/102125
>>> 	* emit-rtl.c (gen_highpart): Use adjust_address to handle
>>> 	MEM rather than calling simplify_gen_subreg.
>>> ---
>>>   gcc/emit-rtl.c | 23 +++++++++++++----------
>>>   1 file changed, 13 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
>>> index 77ea8948ee8..0ba110879aa 100644
>>> --- a/gcc/emit-rtl.c
>>> +++ b/gcc/emit-rtl.c
>>> @@ -1585,19 +1585,22 @@ gen_highpart (machine_mode mode, rtx x)
>>>     gcc_assert (known_le (msize, (unsigned int) UNITS_PER_WORD)
>>>   	      || known_eq (msize, GET_MODE_UNIT_SIZE (GET_MODE (x))));
>>>   
>>> -  result = simplify_gen_subreg (mode, x, GET_MODE (x),
>>> -				subreg_highpart_offset (mode, GET_MODE (x)));
>>> -  gcc_assert (result);
>>> -
>>> -  /* simplify_gen_subreg is not guaranteed to return a valid operand for
>>> -     the target if we have a MEM.  gen_highpart must return a valid operand,
>>> -     emitting code if necessary to do so.  */
>>> -  if (MEM_P (result))
>>> +  /* gen_lowpart_common handles a lot of special cases due to needing to handle
>>> +     paradoxical subregs; it only calls simplify_gen_subreg when certain that
>>> +     it will produce something meaningful.  The only case we need to handle
>>> +     specially here is MEM.  */
>>> +  if (MEM_P (x))
>>>       {
>>> -      result = validize_mem (result);
>>> -      gcc_assert (result);
>>> +      poly_int64 offset = subreg_highpart_offset (mode, GET_MODE (x));
>>> +      return adjust_address (x, mode, offset);
>>>       }
>>>   
>>> +  result = simplify_gen_subreg (mode, x, GET_MODE (x),
>>> +				subreg_highpart_offset (mode, GET_MODE (x)));
>>> +  /* Since we handle MEM directly above, we should never get a MEM back
>>> +     from simplify_gen_subreg.  */
>>> +  gcc_assert (result && !MEM_P (result));
>>> +
>>>     return result;
>>>   }
>>>
diff mbox series

Patch

diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 77ea8948ee8..0ba110879aa 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -1585,19 +1585,22 @@  gen_highpart (machine_mode mode, rtx x)
   gcc_assert (known_le (msize, (unsigned int) UNITS_PER_WORD)
 	      || known_eq (msize, GET_MODE_UNIT_SIZE (GET_MODE (x))));
 
-  result = simplify_gen_subreg (mode, x, GET_MODE (x),
-				subreg_highpart_offset (mode, GET_MODE (x)));
-  gcc_assert (result);
-
-  /* simplify_gen_subreg is not guaranteed to return a valid operand for
-     the target if we have a MEM.  gen_highpart must return a valid operand,
-     emitting code if necessary to do so.  */
-  if (MEM_P (result))
+  /* gen_lowpart_common handles a lot of special cases due to needing to handle
+     paradoxical subregs; it only calls simplify_gen_subreg when certain that
+     it will produce something meaningful.  The only case we need to handle
+     specially here is MEM.  */
+  if (MEM_P (x))
     {
-      result = validize_mem (result);
-      gcc_assert (result);
+      poly_int64 offset = subreg_highpart_offset (mode, GET_MODE (x));
+      return adjust_address (x, mode, offset);
     }
 
+  result = simplify_gen_subreg (mode, x, GET_MODE (x),
+				subreg_highpart_offset (mode, GET_MODE (x)));
+  /* Since we handle MEM directly above, we should never get a MEM back
+     from simplify_gen_subreg.  */
+  gcc_assert (result && !MEM_P (result));
+
   return result;
 }