[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