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

[Jim Grosbach]

In general, physical register references like this are very dangerous and won’t play nicely with the rest of the code. It’s possible the glue here will work around that, but it still seems pretty dodgy. Sometimes when we need an enforced instruction ordering, we use a pseudo-instruction that gets expanded very late (sometimes as late as the MC lowering).

I’m not expert on the assumptions the x86 backend makes about ESI, so I don’t have a very good feel for how safe/unsafe this change really is, unfortunately.

-Jim

On Mar 17, 2014, at 11:41 AM, Hans Wennborg <hans at chromium.org> wrote:

>> 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
> }
> <D2968.2.patch>