[llvm] r204829 - Revert "X86 memcpy lowering: use "rep movs" even when esi is used as base pointer" (r204174)
Hans Wennborg
hans at chromium.org
Wed Mar 26 12:56:58 PDT 2014
I don't have anything handy, this was from a chrome build. I'll try to
get something reduced or some bitcode because I'm curious too :)
On Wed, Mar 26, 2014 at 11:08 AM, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> Do you have a testcase? I am somewhat curious as to what was causing the bug.
>
> On 26 March 2014 12:30, Hans Wennborg <hans at hanshq.net> wrote:
>> Author: hans
>> Date: Wed Mar 26 11:30:54 2014
>> New Revision: 204829
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=204829&view=rev
>> Log:
>> Revert "X86 memcpy lowering: use "rep movs" even when esi is used as 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.
>>
>> Modified:
>> llvm/trunk/lib/Target/X86/X86SelectionDAGInfo.cpp
>> llvm/trunk/test/CodeGen/X86/inline-asm-sp-clobber-memcpy.ll
>> llvm/trunk/test/CodeGen/X86/stack-align-memcpy.ll
>>
>> Modified: llvm/trunk/lib/Target/X86/X86SelectionDAGInfo.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86SelectionDAGInfo.cpp?rev=204829&r1=204828&r2=204829&view=diff
>> ==============================================================================
>> --- llvm/trunk/lib/Target/X86/X86SelectionDAGInfo.cpp (original)
>> +++ llvm/trunk/lib/Target/X86/X86SelectionDAGInfo.cpp Wed Mar 26 11:30:54 2014
>> @@ -200,11 +200,13 @@ X86SelectionDAGInfo::EmitTargetCodeForMe
>> 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)
>> @@ -223,45 +225,27 @@ X86SelectionDAGInfo::EmitTargetCodeForMe
>> 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;
>>
>> Modified: llvm/trunk/test/CodeGen/X86/inline-asm-sp-clobber-memcpy.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/inline-asm-sp-clobber-memcpy.ll?rev=204829&r1=204828&r2=204829&view=diff
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/X86/inline-asm-sp-clobber-memcpy.ll (original)
>> +++ llvm/trunk/test/CodeGen/X86/inline-asm-sp-clobber-memcpy.ll Wed Mar 26 11:30:54 2014
>> @@ -13,7 +13,5 @@ define void @test1(%struct.foo* nocaptur
>>
>> ; CHECK-LABEL: test1:
>> ; CHECK: movl %esp, %esi
>> -; CHECK: movl %esi, %edx
>> -; CHECK: rep;movsl
>> -; CHECK: movl %edx, %esi
>> +; CHECK-NOT: rep;movsl
>> }
>>
>> Modified: llvm/trunk/test/CodeGen/X86/stack-align-memcpy.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/stack-align-memcpy.ll?rev=204829&r1=204828&r2=204829&view=diff
>> ==============================================================================
>> --- llvm/trunk/test/CodeGen/X86/stack-align-memcpy.ll (original)
>> +++ llvm/trunk/test/CodeGen/X86/stack-align-memcpy.ll Wed Mar 26 11:30:54 2014
>> @@ -15,9 +15,7 @@ define void @test1(%struct.foo* nocaptur
>> ; 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
>> @@ -30,9 +28,7 @@ define void @test2(%struct.foo* nocaptur
>>
>> ; 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.
>> @@ -43,6 +39,5 @@ define void @test3(%struct.foo* nocaptur
>> ret void
>>
>> ; CHECK-LABEL: test3:
>> -; CHECK-NOT: movl %esi, %edx
>> ; CHECK: rep;movsl
>> }
More information about the llvm-commits
mailing list