[PATCH] Clean up by_pieces_ninsns

[Richard Sandiford]

"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;
>>