[PATCH] [AArch64] Inline memcpy() as a sequence of ldp-stp with 64-bit registers

[James Molloy]

Hi,

I think the principle here is OK. It'd have been nicer if we could convince the scheduler to do this instead, rather than going behind its back though. Have you talked to Andy Trick or Dave Estes to work out if this is possible?

Comments inline.

I'd also like Tim's signoff before this goes in.

James

================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:474
@@ +473,3 @@
+    // instructions operating on 128-bit registers.  Allow twice as big
+    // instructions af for memmove().
+    MaxStoresPerMemcpy = MaxStoresPerMemcpyOptSize = 8;
----------------
s/af/as

================
Comment at: lib/Target/AArch64/AArch64LoadStoreInterleave.cpp:25
@@ +24,3 @@
+//              2. <something1>              2. <something2>
+//              3. <store1>                  3. <load1>
+//              4. <something2>              4. <store1>
----------------
The important thing is that we have ldp/stp in that order, ideally with increasing addresses. We don't need to cluster them all together - it's the ordering of memory operations that counts I think.

So we can have:

ldp
stp
add # unrelated operation
ldp
stp

This should be fine, and may be a good thing, depending on the microarchitecture. 

================
Comment at: lib/Target/AArch64/AArch64LoadStoreInterleave.cpp:48
@@ +47,3 @@
+
+STATISTIC(NumSequences, "Number of load/pair sequences updated");
+
----------------
This is a fairly generic statistic name. Something more A64 specific perhaps?

================
Comment at: lib/Target/AArch64/AArch64LoadStoreInterleave.cpp:144
@@ +143,3 @@
+static bool isSafeInstruction(unsigned LdBase, unsigned StBase, MachineInstr *I,
+                              const TargetRegisterInfo *TRI, int SeenStore) {
+  if (I->isDebugValue()) {
----------------
Wouldn't isSafeToSpeculate() conservatively do the same job here?

================
Comment at: lib/Target/AArch64/AArch64LoadStoreInterleave.cpp:205
@@ +204,3 @@
+    case AArch64::STURXi:
+      // Loads should go first.
+      if (!Lds.empty()) {
----------------
This was not mentioned in the comment; why should all loads come before all stores?

================
Comment at: lib/Target/AArch64/AArch64LoadStoreInterleave.cpp:290
@@ +289,3 @@
+
+    if (std::find(Lds.begin(), Lds.end(), (MachineInstr*)I) != Lds.end()) {
+      continue;
----------------
Here and elsewhere: single-line if's should have their {}'s removed.

http://reviews.llvm.org/D6054