"Kewen.Lin" <li...@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;
>>
