[PATCH][AArch64] Use 8-byte load&store for inlined memcpy() on Cortex A53

[James Molloy]

Hi Sergey,

Also, generated code doesn't always like "ldp; stp; ldp; stp", but can
> be either of form:
>   ldp
>   ...something...
>   stp
> or:
>   ldp
>   ldp
>   stp
>   stp
> Is it OK for performance?


I'm afraid that would be a performance regression. That sequence requires
twice the number of live registers. For performance it is important to have
the ldp and stp properly interleaved, both on A57 and A53.

I also suspect that f128 logic was put in for Cyclone so Apple might get
annoyed if you remove it ;)

Cheers,

James


On 16 July 2014 15:33, Sergey Dmitrouk <sdmitrouk at accesssoftek.com> wrote:

> Hi again,
>
> I was going through code looking what's wrong with generic load&store
> generation, but found the issue in test.  Using "undef" as destination
> for "llvm.memcpy" as it was in other test is bad idea, code generator
> actually produces meaningless code in this case.  Destination pointer
> is not advanced for store instructions (offset is just ignored), which
> prevents ldp/stp pass to squish sequences of ldr/str instructions.
>
> So I fixed the test and this time provide two patches: one for
> Cortex-A53 only and the other one for all subtargets.  Take a look at
> the second one, I don't really like that it basically removes code for
> 128-bit types, maybe there is a reason why it was put there.
>
> Also, generated code doesn't always like "ldp; stp; ldp; stp", but can
> be either of form:
>   ldp
>   ...something...
>   stp
> or:
>   ldp
>   ldp
>   stp
>   stp
> Is it OK for performance?
>
> Regards,
> Sergey
>
> On Wed, Jul 16, 2014 at 12:08:51AM +0300, Sergey Dmitrouk wrote:
> > Hi James,
> >
> > Sorry, I missed that difference.  It shouldn't be hard to do that ideal
> > sequence for all targets though.  One should provide custom
> > implementation for AArch64SelectionDAGInfo::EmitTargetCodeForMemcpy,
> which
> > was my initial way of doing this task.  Code in that callback can
> generate
> > any instructions, so I'll try to do that.
> >
> > The only issue is that SelectionDAG prefers its own load&store generator
> and
> > invokes it before the custom one.  I don't know a good way to overcome
> > that at the moment.  It's possible to set unreasonable limits for
> > instructions emitted by generic load&store implementation, which will
> cause
> > SelectionDAG to reject it, but it's a hack, which I'd prefer not to
> > implement.  There should be a better way.
> >
> > Thanks & Cheers,
> > Sergey
> >
> > On Tue, Jul 15, 2014 at 12:54:52PM -0700, James Molloy wrote:
> > >    Hi Sergey,
> > >    Thanks for working on this! The answer is slightly more involved
> though, I
> > >    think.
> > >    As shown in your testcase, your change emits the sequence "ldr;
> str; ldr;
> > >    str". The ideal expansion is "ldp; stp; ldp; stp;". That way we
> still do
> > >    128-bit loads and stores.
> > >    In fact, our microarchitects have recommended (through internal
> channels)
> > >    that the "ldp; stp" sequence be used for memcpy-like operations -
> this
> > >    will give portable performance. Therefore, the change should also
> be made
> > >    for at least A57. I'll let Tim or Jim comment on Cyclone.
> > >    So to generate "ldp stp", the inline memcpy expander needs to
> generate
> > >    "ldr; ldr; str; str;". The ldp/stp pass will then squish these
> together.
> > >    A similar thing is done in the ARM target (which gets combined into
> LDRD
> > >    or LDM), but it's ARM-only. I think some logic needs to me moved
> into the
> > >    target-independent part of codegen.
> > >    Cheers,
> > >    James
> > >
> > >    On 15 July 2014 09:15, Sergey Dmitrouk <sdmitrouk at accesssoftek.com>
> wrote:
> > >
> > >      Hi,
> > >
> > >      Basing on the following information from [this post][0] by James
> Molloy:
> > >
> > >      A  * Our inline memcpy expansion pass is emitting "LDR q0, [..];
> STR q0,
> > >      A  [..]" pairs, which is less than ideal on A53. If we switched to
> > >      A  emitting "LDP x0, x1, [..]; STP x0, x1, [..]", we'd get around
> 30%
> > >      A  better inline memcpy performance on A53. A57 seems to deal
> well with
> > >      A  the LDR q sequence.
> > >
> > >      I've made a patch (attached) that does this for Cortex-A53. A
> Please
> > >      take a look at it.
> > >
> > >      Best regards,
> > >      Sergey
> > >
> > >      0: http://article.gmane.org/gmane.comp.compilers.llvm.devel/74269
> > >
> > >      _______________________________________________
> > >      llvm-commits mailing list
> > >      llvm-commits at cs.uiuc.edu
> > >      http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140716/a2cd34a4/attachment.html>