Re: [PATCH] MIPS: don't expand large block move

[YunQiang Su <syq@debian.org>]

Maciej W. Rozycki <macro@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