[PATCH] memcpy lowering: use "rep movs" even when esi is used as base pointer

[Hans Wennborg]

  > The existing code is odd.

  Do you mean the InFlag name or something else?

  > The "flag" has been called glue since r122310, so renaming the
  > variable might be a nice first cleanup :-). There seems to be more
  > glue than it is necessary, but that is probably not an issue.

  I've renamed it.

  > I also see llc producing
  >
  > movl 8(%ebp), %esi
  > leal 4(%esp), %edi
  > movl $22, %ecx
  > rep;movsl
  >
  > Which seems to be the exact opposite of what the glue links would
  > imply. They are initially created in the correct order

  I noticed this too, but I figured it was OK. The way I think about it (which could be completely wrong) is that the glue ties down the order of MI instructions when they're generated, but after that the glue is gone and the MI passes can reorder instructions if they're confident that it's safe. I imagine that's what's happening here.

Hi rafael,

http://llvm-reviews.chandlerc.com/D2968

CHANGE SINCE LAST DIFF
  http://llvm-reviews.chandlerc.com/D2968?vs=7557&id=7892#toc

Files:
  lib/Target/X86/X86SelectionDAGInfo.cpp
  test/CodeGen/X86/inline-asm-sp-clobber-memcpy.ll
  test/CodeGen/X86/stack-align-memcpy.ll

Index: lib/Target/X86/X86SelectionDAGInfo.cpp
===================================================================
--- lib/Target/X86/X86SelectionDAGInfo.cpp
+++ lib/Target/X86/X86SelectionDAGInfo.cpp
@@ -200,13 +200,11 @@
       SrcPtrInfo.getAddrSpace() >= 256)
     return SDValue();
 
-  // ESI might be used as a base pointer, in that case we can't simply overwrite
-  // the register.  Fall back to generic code.
+  // If ESI is used as a base pointer, we must preserve it when doing rep movs.
   const X86RegisterInfo *TRI =
       static_cast<const X86RegisterInfo *>(DAG.getTarget().getRegisterInfo());
-  if (TRI->hasBasePointer(DAG.getMachineFunction()) &&
-      TRI->getBaseRegister() == X86::ESI)
-    return SDValue();
+  bool PreserveESI = TRI->hasBasePointer(DAG.getMachineFunction()) &&
+                     TRI->getBaseRegister() == X86::ESI;
 
   MVT AVT;
   if (Align & 1)
@@ -225,27 +223,43 @@
   SDValue Count = DAG.getIntPtrConstant(CountVal);
   unsigned BytesLeft = SizeVal % UBytes;
 
-  SDValue InFlag(0, 0);
+  SDValue InGlue(0, 0);
+
+  if (PreserveESI) {
+    // Save ESI to a physical register. (We cannot use a virtual register
+    // because if it is spilled we wouldn't be ablle to reload it.)
+    Chain = DAG.getCopyToReg(Chain, dl, X86::EDX,
+                             DAG.getRegister(X86::ESI, MVT::i32), InGlue);
+  }
+
   Chain  = DAG.getCopyToReg(Chain, dl, Subtarget->is64Bit() ? X86::RCX :
                                                               X86::ECX,
-                            Count, InFlag);
-  InFlag = Chain.getValue(1);
+                            Count, InGlue);
+  InGlue = Chain.getValue(1);
   Chain  = DAG.getCopyToReg(Chain, dl, Subtarget->is64Bit() ? X86::RDI :
                                                               X86::EDI,
-                            Dst, InFlag);
-  InFlag = Chain.getValue(1);
+                            Dst, InGlue);
+  InGlue = Chain.getValue(1);
   Chain  = DAG.getCopyToReg(Chain, dl, Subtarget->is64Bit() ? X86::RSI :
                                                               X86::ESI,
-                            Src, InFlag);
-  InFlag = Chain.getValue(1);
+                            Src, InGlue);
+  InGlue = Chain.getValue(1);
 
   SDVTList Tys = DAG.getVTList(MVT::Other, MVT::Glue);
-  SDValue Ops[] = { Chain, DAG.getValueType(AVT), InFlag };
+  SDValue Ops[] = { Chain, DAG.getValueType(AVT), InGlue };
   SDValue RepMovs = DAG.getNode(X86ISD::REP_MOVS, dl, Tys, Ops,
                                 array_lengthof(Ops));
 
+  if (PreserveESI) {
+    InGlue = RepMovs.getValue(1);
+    RepMovs = DAG.getCopyToReg(RepMovs, dl, X86::ESI,
+                               DAG.getRegister(X86::EDX, MVT::i32), InGlue);
+  }
+
   SmallVector<SDValue, 4> Results;
   Results.push_back(RepMovs);
+
+
   if (BytesLeft) {
     // Handle the last 1 - 7 bytes.
     unsigned Offset = SizeVal - BytesLeft;
Index: test/CodeGen/X86/inline-asm-sp-clobber-memcpy.ll
===================================================================
--- test/CodeGen/X86/inline-asm-sp-clobber-memcpy.ll
+++ test/CodeGen/X86/inline-asm-sp-clobber-memcpy.ll
@@ -13,5 +13,7 @@
 
 ; CHECK-LABEL: test1:
 ; CHECK: movl %esp, %esi
-; CHECK-NOT: rep;movsl
+; CHECK: movl %esi, %edx
+; CHECK: rep;movsl
+; CHECK: movl %edx, %esi
 }
Index: test/CodeGen/X86/stack-align-memcpy.ll
===================================================================
--- test/CodeGen/X86/stack-align-memcpy.ll
+++ test/CodeGen/X86/stack-align-memcpy.ll
@@ -15,7 +15,9 @@
 ; CHECK-LABEL: test1:
 ; CHECK: andl $-16, %esp
 ; CHECK: movl %esp, %esi
-; CHECK-NOT: rep;movsl
+; CHECK: movl %esi, %edx
+; CHECK: rep;movsl
+; CHECK: movl %edx, %esi
 }
 
 ; PR19012
@@ -28,7 +30,9 @@
 
 ; CHECK-LABEL: test2:
 ; CHECK: movl %esp, %esi
-; CHECK-NOT: rep;movsl
+; CHECK: movl %esi, %edx
+; CHECK: rep;movsl
+; CHECK: movl %edx, %esi
 }
 
 ; Check that we do use rep movs if we make the alloca static.
@@ -39,5 +43,6 @@
   ret void
 
 ; CHECK-LABEL: test3:
+; CHECK-NOT: movl %esi, %edx
 ; CHECK: rep;movsl
 }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D2968.2.patch
Type: text/x-patch
Size: 4068 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140317/11b13e72/attachment.bin>