Skip to content

Commit

Permalink
Revert "X86 memcpy lowering: use "rep movs" even when esi is used as …
Browse files Browse the repository at this point in the history
…base pointer" (r204174)

>  For functions where esi is used as base pointer, we would previously fall ba
>  from lowering memcpy with "rep movs" because that clobbers esi.
>
>  With this patch, we just store esi in another physical register, and restore
>  it afterwards. This adds a little bit of register preassure, but the more
>  efficient memcpy should be worth it.
>
>  Differential Revision: http://llvm-reviews.chandlerc.com/D2968

This didn't work. I was ending up with code like this:

  lea     edi,[esi+38h]
  mov     ecx,0Fh
  mov     edx,esi
  mov     esi,ebx
  rep movs dword ptr es:[edi],dword ptr [esi]
  lea     ecx,[esi+74h] <-- Ooops, we're now using esi before restoring it from edx.
  add     ebx,3Ch
  mov     esi,edx

I guess if we want to do this we need stronger glue or something, or doing the expansion
much later.

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@204829 91177308-0d34-0410-b5e6-96231b3b80d8
  • Loading branch information
zmodem committed Mar 26, 2014
1 parent 7363d22 commit b82f8a2
Show file tree
Hide file tree
Showing 3 changed files with 16 additions and 39 deletions.
42 changes: 13 additions & 29 deletions lib/Target/X86/X86SelectionDAGInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -200,11 +200,13 @@ X86SelectionDAGInfo::EmitTargetCodeForMemcpy(SelectionDAG &DAG, SDLoc dl,
SrcPtrInfo.getAddrSpace() >= 256)
return SDValue();

// If ESI is used as a base pointer, we must preserve it when doing rep movs.
// ESI might be used as a base pointer, in that case we can't simply overwrite
// the register. Fall back to generic code.
const X86RegisterInfo *TRI =
static_cast<const X86RegisterInfo *>(DAG.getTarget().getRegisterInfo());
bool PreserveESI = TRI->hasBasePointer(DAG.getMachineFunction()) &&
TRI->getBaseRegister() == X86::ESI;
if (TRI->hasBasePointer(DAG.getMachineFunction()) &&
TRI->getBaseRegister() == X86::ESI)
return SDValue();

MVT AVT;
if (Align & 1)
Expand All @@ -223,45 +225,27 @@ X86SelectionDAGInfo::EmitTargetCodeForMemcpy(SelectionDAG &DAG, SDLoc dl,
SDValue Count = DAG.getIntPtrConstant(CountVal);
unsigned BytesLeft = SizeVal % UBytes;


if (PreserveESI) {
// Save ESI to a physical register. (We cannot use a virtual register
// because if it is spilled we wouldn't be able to reload it.)
// We don't glue this because the register dependencies are explicit.
Chain = DAG.getCopyToReg(Chain, dl, X86::EDX,
DAG.getRegister(X86::ESI, MVT::i32));
}

SDValue InGlue(0, 0);
SDValue InFlag(0, 0);
Chain = DAG.getCopyToReg(Chain, dl, Subtarget->is64Bit() ? X86::RCX :
X86::ECX,
Count, InGlue);
InGlue = Chain.getValue(1);
Count, InFlag);
InFlag = Chain.getValue(1);
Chain = DAG.getCopyToReg(Chain, dl, Subtarget->is64Bit() ? X86::RDI :
X86::EDI,
Dst, InGlue);
InGlue = Chain.getValue(1);
Dst, InFlag);
InFlag = Chain.getValue(1);
Chain = DAG.getCopyToReg(Chain, dl, Subtarget->is64Bit() ? X86::RSI :
X86::ESI,
Src, InGlue);
InGlue = Chain.getValue(1);
Src, InFlag);
InFlag = Chain.getValue(1);

SDVTList Tys = DAG.getVTList(MVT::Other, MVT::Glue);
SDValue Ops[] = { Chain, DAG.getValueType(AVT), InGlue };
// FIXME: Make X86rep_movs explicitly use FCX, RDI, RSI instead of glue.
SDValue Ops[] = { Chain, DAG.getValueType(AVT), InFlag };
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;
Expand Down
4 changes: 1 addition & 3 deletions test/CodeGen/X86/inline-asm-sp-clobber-memcpy.ll
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,5 @@ define void @test1(%struct.foo* nocapture %x, i32 %y, i8* %z) nounwind {

; CHECK-LABEL: test1:
; CHECK: movl %esp, %esi
; CHECK: movl %esi, %edx
; CHECK: rep;movsl
; CHECK: movl %edx, %esi
; CHECK-NOT: rep;movsl
}
9 changes: 2 additions & 7 deletions test/CodeGen/X86/stack-align-memcpy.ll
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ define void @test1(%struct.foo* nocapture %x, i32 %y) nounwind {
; CHECK-LABEL: test1:
; CHECK: andl $-16, %esp
; CHECK: movl %esp, %esi
; CHECK: movl %esi, %edx
; CHECK: rep;movsl
; CHECK: movl %edx, %esi
; CHECK-NOT: rep;movsl
}

; PR19012
Expand All @@ -30,9 +28,7 @@ define void @test2(%struct.foo* nocapture %x, i32 %y, i8* %z) nounwind {

; CHECK-LABEL: test2:
; CHECK: movl %esp, %esi
; CHECK: movl %esi, %edx
; CHECK: rep;movsl
; CHECK: movl %edx, %esi
; CHECK-NOT: rep;movsl
}

; Check that we do use rep movs if we make the alloca static.
Expand All @@ -43,6 +39,5 @@ define void @test3(%struct.foo* nocapture %x, i32 %y, i8* %z) nounwind {
ret void

; CHECK-LABEL: test3:
; CHECK-NOT: movl %esi, %edx
; CHECK: rep;movsl
}

0 comments on commit b82f8a2

Please sign in to comment.