Maciej W. Rozycki <ma...@orcam.me.uk> 于2023年5月20日周六 03:21写道：
>
> On Fri, 19 May 2023, Jeff Law wrote:
>
> > > diff --git a/gcc/config/mips/mips.cc b/gcc/config/mips/mips.cc
> > > index ca491b981a3..00f26d5e923 100644
> > > --- a/gcc/config/mips/mips.cc
> > > +++ b/gcc/config/mips/mips.cc
> > > @@ -8313,6 +8313,12 @@ mips_expand_block_move (rtx dest, rtx src, rtx
> > > length)
> > > }
> > > else if (optimize)
> > > {
> > > + /* When the length is big enough, the lib call has better performace
> > > + than load/store insns.
> > > + In most platform, the value is about 64-128.
> > > + And in fact lib call may be optimized with SIMD */
> > > + if (INTVAL(length) >= 64)
> > > + return false;
> > Just a formatting nit. Space between INTVAL and the open paren for its
> > argument list.
>
> This is oddly wrapped too. I'd move "performace" (typo there!) to the
> second line, to align better with the rest of the text.
>
> Plus s/platform/platforms/ and there's a full stop missing along with two
> spaces at the end. Also there's inconsistent style around <= and >=; the
> GNU Coding Standards ask for spaces around binary operators. And "don't"
> in the change heading ought to be capitalised.
>
> In fact, I'd justify the whole paragraph as each sentence doesn't have to
> start on a new line, and the commit description could benefit from some
> reformatting too, as it's now odd to read.
>
Thank you. I will fix these problems.
> > OK with that change.
>
> I think the conditional would be better readable if it was flattened
> though:
>
> if (INTVAL (length) <= MIPS_MAX_MOVE_BYTES_STRAIGHT)
> ...
> else if (INTVAL (length) >= 64)
> ...
> else if (optimize)
> ...
>
This sounds good.
> or even:
>
> if (INTVAL (length) <= MIPS_MAX_MOVE_BYTES_STRAIGHT)
> ...
> else if (INTVAL (length) < 64 && optimize)
> ...
>
I don't think this is a good option, since somebody may add some code,
and may break our logic.
> One just wouldn't write it as proposed if creating the whole piece from
> scratch rather than retrofitting this extra conditional.
>
> Ultimately it may have to be tunable as LWL/LWR, etc. may be subject to
> fusion and may be faster after all.
>
oohhh, you are right.
And in fact this patch has some problems:
If the data is aligned, the value is about 1024, instead of 64-128.
> Maciej
