| Message ID | 87edf7bc-1096-486d-a029-f6a3450739c9@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean up by_pieces_ninsns | expand |
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; >
"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 --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;