Re: [PATCH] Clean up by_pieces_ninsns
"Kewen.Lin" 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;
>>
Re: [PATCH] Clean up by_pieces_ninsns
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;
>
[PATCH] Clean up by_pieces_ninsns
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)
- {
- 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;
