diff mbox series

Clean up by_pieces_ninsns

Message ID 87edf7bc-1096-486d-a029-f6a3450739c9@linux.ibm.com
State Unresolved
Headers
Series Clean up by_pieces_ninsns |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

HAO CHEN GUI Nov. 15, 2023, 2:26 a.m. UTC 
  Hi,
  This patch cleans up by_pieces_ninsns and does following things.
1. Do the length and alignment adjustment for by pieces compare when
overlap operation is enabled.
2. Remove unnecessary mov_optab checks.

  Bootstrapped and tested on x86 and powerpc64-linux BE and LE with
no regressions. Is this OK for trunk?

Thanks
Gui Haochen

ChangeLog
Clean up by_pieces_ninsns

The by pieces compare can be implemented by overlapped operations. So
it should be taken into consideration when doing the adjustment for
overlap operations.  The mode returned from
widest_fixed_size_mode_for_size is already checked with mov_optab in
by_pieces_mode_supported_p called by widest_fixed_size_mode_for_size.
So there is no need to check mov_optab again in by_pieces_ninsns.
The patch fixes these issues.

gcc/
	* expr.cc (by_pieces_ninsns): Include by pieces compare when
	do the adjustment for overlap operations.  Remove unnecessary
	mov_optab check.

patch.diff

Comments

Kewen.Lin Nov. 15, 2023, 8:41 a.m. UTC | #1 
Hi,

on 2023/11/15 10:26, HAO CHEN GUI wrote:
> Hi,
>   This patch cleans up by_pieces_ninsns and does following things.
> 1. Do the length and alignment adjustment for by pieces compare when
> overlap operation is enabled.
> 2. Remove unnecessary mov_optab checks.
> 
>   Bootstrapped and tested on x86 and powerpc64-linux BE and LE with
> no regressions. Is this OK for trunk?
> 
> Thanks
> Gui Haochen
> 
> ChangeLog
> Clean up by_pieces_ninsns
> 
> The by pieces compare can be implemented by overlapped operations. So
> it should be taken into consideration when doing the adjustment for
> overlap operations.  The mode returned from
> widest_fixed_size_mode_for_size is already checked with mov_optab in
> by_pieces_mode_supported_p called by widest_fixed_size_mode_for_size.
> So there is no need to check mov_optab again in by_pieces_ninsns.
> The patch fixes these issues.
> 
> gcc/
> 	* expr.cc (by_pieces_ninsns): Include by pieces compare when
> 	do the adjustment for overlap operations.  Remove unnecessary
> 	mov_optab check.
> 
> patch.diff
> diff --git a/gcc/expr.cc b/gcc/expr.cc
> index 3e2a678710d..7cb2c935177 100644
> --- a/gcc/expr.cc
> +++ b/gcc/expr.cc
> @@ -1090,18 +1090,15 @@ by_pieces_ninsns (unsigned HOST_WIDE_INT l, unsigned int align,
>    unsigned HOST_WIDE_INT n_insns = 0;
>    fixed_size_mode mode;
> 
> -  if (targetm.overlap_op_by_pieces_p () && op != COMPARE_BY_PIECES)
> +  if (targetm.overlap_op_by_pieces_p ())
>      {
>        /* NB: Round up L and ALIGN to the widest integer mode for
>  	 MAX_SIZE.  */
>        mode = widest_fixed_size_mode_for_size (max_size, op);
> -      if (optab_handler (mov_optab, mode) != CODE_FOR_nothing)

These changes are on generic code, so not a review.  :)

If it's guaranteed previously, maybe we can replace it with an assertion
like: gcc_assert (optab_handler (mov_optab, mode) != CODE_FOR_nothing);

> -	{
> -	  unsigned HOST_WIDE_INT up = ROUND_UP (l, GET_MODE_SIZE (mode));
> -	  if (up > l)
> -	    l = up;
> -	  align = GET_MODE_ALIGNMENT (mode);
> -	}
> +      unsigned HOST_WIDE_INT up = ROUND_UP (l, GET_MODE_SIZE (mode));
> +	if (up > l)
> +	  l = up;
> +      align = GET_MODE_ALIGNMENT (mode);
>      }
> 
>    align = alignment_for_piecewise_move (MOVE_MAX_PIECES, align);
> @@ -1109,12 +1106,10 @@ by_pieces_ninsns (unsigned HOST_WIDE_INT l, unsigned int align,
>    while (max_size > 1 && l > 0)
>      {
>        mode = widest_fixed_size_mode_for_size (max_size, op);
> -      enum insn_code icode;
> 
>        unsigned int modesize = GET_MODE_SIZE (mode);
> 
> -      icode = optab_handler (mov_optab, mode);

... likewise.

BR,
Kewen

> -      if (icode != CODE_FOR_nothing && align >= GET_MODE_ALIGNMENT (mode))
> +      if (align >= GET_MODE_ALIGNMENT (mode))
>  	{
>  	  unsigned HOST_WIDE_INT n_pieces = l / modesize;
>  	  l %= modesize;
>
Richard Sandiford Nov. 22, 2023, 6:33 p.m. UTC | #2 
"Kewen.Lin" <linkw@linux.ibm.com> writes:
> Hi,
>
> on 2023/11/15 10:26, HAO CHEN GUI wrote:
>> Hi,
>>   This patch cleans up by_pieces_ninsns and does following things.
>> 1. Do the length and alignment adjustment for by pieces compare when
>> overlap operation is enabled.
>> 2. Remove unnecessary mov_optab checks.
>> 
>>   Bootstrapped and tested on x86 and powerpc64-linux BE and LE with
>> no regressions. Is this OK for trunk?
>> 
>> Thanks
>> Gui Haochen
>> 
>> ChangeLog
>> Clean up by_pieces_ninsns
>> 
>> The by pieces compare can be implemented by overlapped operations. So
>> it should be taken into consideration when doing the adjustment for
>> overlap operations.  The mode returned from
>> widest_fixed_size_mode_for_size is already checked with mov_optab in
>> by_pieces_mode_supported_p called by widest_fixed_size_mode_for_size.
>> So there is no need to check mov_optab again in by_pieces_ninsns.
>> The patch fixes these issues.
>> 
>> gcc/
>> 	* expr.cc (by_pieces_ninsns): Include by pieces compare when
>> 	do the adjustment for overlap operations.  Remove unnecessary
>> 	mov_optab check.
>> 
>> patch.diff
>> diff --git a/gcc/expr.cc b/gcc/expr.cc
>> index 3e2a678710d..7cb2c935177 100644
>> --- a/gcc/expr.cc
>> +++ b/gcc/expr.cc
>> @@ -1090,18 +1090,15 @@ by_pieces_ninsns (unsigned HOST_WIDE_INT l, unsigned int align,
>>    unsigned HOST_WIDE_INT n_insns = 0;
>>    fixed_size_mode mode;
>> 
>> -  if (targetm.overlap_op_by_pieces_p () && op != COMPARE_BY_PIECES)
>> +  if (targetm.overlap_op_by_pieces_p ())
>>      {
>>        /* NB: Round up L and ALIGN to the widest integer mode for
>>  	 MAX_SIZE.  */
>>        mode = widest_fixed_size_mode_for_size (max_size, op);
>> -      if (optab_handler (mov_optab, mode) != CODE_FOR_nothing)
>
> These changes are on generic code, so not a review.  :)
>
> If it's guaranteed previously, maybe we can replace it with an assertion
> like: gcc_assert (optab_handler (mov_optab, mode) != CODE_FOR_nothing);

Yeah, sounds OK to me FWIW.  I suppose the counter-argument is that
nothing here directly relies on the move optab.  It's just checking on
behalf of later code, which is now done by widest_fixed_size_mode_for_size
instead.

So the patch as posted is OK for trunk too, except that:

>
>> -	{
>> -	  unsigned HOST_WIDE_INT up = ROUND_UP (l, GET_MODE_SIZE (mode));
>> -	  if (up > l)
>> -	    l = up;
>> -	  align = GET_MODE_ALIGNMENT (mode);
>> -	}
>> +      unsigned HOST_WIDE_INT up = ROUND_UP (l, GET_MODE_SIZE (mode));
>> +	if (up > l)
>> +	  l = up;
>> +      align = GET_MODE_ALIGNMENT (mode);

the indentation looks off here (the "if" is indented differently from the
first and last statements).

Thanks,
Richard

>>      }
>> 
>>    align = alignment_for_piecewise_move (MOVE_MAX_PIECES, align);
>> @@ -1109,12 +1106,10 @@ by_pieces_ninsns (unsigned HOST_WIDE_INT l, unsigned int align,
>>    while (max_size > 1 && l > 0)
>>      {
>>        mode = widest_fixed_size_mode_for_size (max_size, op);
>> -      enum insn_code icode;
>> 
>>        unsigned int modesize = GET_MODE_SIZE (mode);
>> 
>> -      icode = optab_handler (mov_optab, mode);
>
> ... likewise.
>
> BR,
> Kewen
>
>> -      if (icode != CODE_FOR_nothing && align >= GET_MODE_ALIGNMENT (mode))
>> +      if (align >= GET_MODE_ALIGNMENT (mode))
>>  	{
>>  	  unsigned HOST_WIDE_INT n_pieces = l / modesize;
>>  	  l %= modesize;
>>
diff mbox series

Patch

diff --git a/gcc/expr.cc b/gcc/expr.cc
index 3e2a678710d..7cb2c935177 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -1090,18 +1090,15 @@  by_pieces_ninsns (unsigned HOST_WIDE_INT l, unsigned int align,
   unsigned HOST_WIDE_INT n_insns = 0;
   fixed_size_mode mode;

-  if (targetm.overlap_op_by_pieces_p () && op != COMPARE_BY_PIECES)
+  if (targetm.overlap_op_by_pieces_p ())
     {
       /* NB: Round up L and ALIGN to the widest integer mode for
 	 MAX_SIZE.  */
       mode = widest_fixed_size_mode_for_size (max_size, op);
-      if (optab_handler (mov_optab, mode) != CODE_FOR_nothing)
-	{
-	  unsigned HOST_WIDE_INT up = ROUND_UP (l, GET_MODE_SIZE (mode));
-	  if (up > l)
-	    l = up;
-	  align = GET_MODE_ALIGNMENT (mode);
-	}
+      unsigned HOST_WIDE_INT up = ROUND_UP (l, GET_MODE_SIZE (mode));
+	if (up > l)
+	  l = up;
+      align = GET_MODE_ALIGNMENT (mode);
     }

   align = alignment_for_piecewise_move (MOVE_MAX_PIECES, align);
@@ -1109,12 +1106,10 @@  by_pieces_ninsns (unsigned HOST_WIDE_INT l, unsigned int align,
   while (max_size > 1 && l > 0)
     {
       mode = widest_fixed_size_mode_for_size (max_size, op);
-      enum insn_code icode;

       unsigned int modesize = GET_MODE_SIZE (mode);

-      icode = optab_handler (mov_optab, mode);
-      if (icode != CODE_FOR_nothing && align >= GET_MODE_ALIGNMENT (mode))
+      if (align >= GET_MODE_ALIGNMENT (mode))
 	{
 	  unsigned HOST_WIDE_INT n_pieces = l / modesize;
 	  l %= modesize;