X86: Fix conflict over ESI between base register and rep;movsl

[rnk]

Summary

The "solution" is to not use this lowering if there are any dynamic
allocas in the current function. We know up front if there are dynamic
allocas, but we don't know if we'll need to create stack temporaries
with large alignment during lowering. Conservatively assume that we will
need such temporaries.

[hans]

LGTM!

I'm still planning to retry the "store ESI in another physreg while rep movs'ing" approach instead of bailing out, but that's orthogonal to this fix.

[rnk]

Chandler suggested creating an opaque DAG node and lowering it in MI
*after* ISel. The MI layer is much less fluid than the DAG.

[rnk]

Closed by commit rL216775 (authored by @rnk).

lib/Target/X86/X86SelectionDAGInfo.h

 
 namespace llvm {
 
 class X86TargetLowering;
 class X86TargetMachine;
 class X86Subtarget;
 
 class X86SelectionDAGInfo : public TargetSelectionDAGInfo {
+  /// Returns true if it is possible for the base register to conflict with the
+  /// given set of clobbers for a memory intrinsic.
+  bool isBaseRegConflictPossible(SelectionDAG &DAG,
+                                 ArrayRef<unsigned> ClobberSet) const;
+
 public:
   explicit X86SelectionDAGInfo(const DataLayout &DL);
   ~X86SelectionDAGInfo();
 
   SDValue EmitTargetCodeForMemset(SelectionDAG &DAG, SDLoc dl,
                                   SDValue Chain,
                                   SDValue Dst, SDValue Src,
                                   SDValue Size, unsigned Align,

lib/Target/X86/X86SelectionDAGInfo.cpp

 
 #define DEBUG_TYPE "x86-selectiondag-info"
 
 X86SelectionDAGInfo::X86SelectionDAGInfo(const DataLayout &DL)
     : TargetSelectionDAGInfo(&DL) {}
 
 X86SelectionDAGInfo::~X86SelectionDAGInfo() {}
 
+bool X86SelectionDAGInfo::isBaseRegConflictPossible(
+    SelectionDAG &DAG, ArrayRef<unsigned> ClobberSet) const {
+  // We cannot use TRI->hasBasePointer() until *after* we select all basic
+  // blocks.  Legalization may introduce new stack temporaries with large
+  // alignment requirements.  Fall back to generic code if there are any
+  // dynamic stack adjustments (hopefully rare) and the base pointer would
+  // conflict if we had to use it.
+  MachineFrameInfo *MFI = DAG.getMachineFunction().getFrameInfo();
+  if (!MFI->hasVarSizedObjects() && !MFI->hasInlineAsmWithSPAdjust())
+    return false;
+
+  const X86RegisterInfo *TRI = static_cast<const X86RegisterInfo *>(
+      DAG.getSubtarget().getRegisterInfo());
+  unsigned BaseReg = TRI->getBaseRegister();
+  for (unsigned R : ClobberSet)
+    if (BaseReg == R)
+      return true;
+  return false;
+}
+
 SDValue
 X86SelectionDAGInfo::EmitTargetCodeForMemset(SelectionDAG &DAG, SDLoc dl,
                                              SDValue Chain,
                                              SDValue Dst, SDValue Src,
                                              SDValue Size, unsigned Align,
                                              bool isVolatile,
                                          MachinePointerInfo DstPtrInfo) const {
   ConstantSDNode *ConstantSize = dyn_cast<ConstantSDNode>(Size);
   const X86Subtarget &Subtarget = DAG.getTarget().getSubtarget<X86Subtarget>();
 
+#ifndef NDEBUG
+  // If the base register might conflict with our physical registers, bail out.
+  unsigned ClobberSet[] = {X86::RCX, X86::RAX, X86::RDI,
+                           X86::ECX, X86::EAX, X86::EDI};
+  assert(!isBaseRegConflictPossible(DAG, ClobberSet));
+#endif
+
   // If to a segment-relative address space, use the default lowering.
   if (DstPtrInfo.getAddrSpace() >= 256)
     return SDValue();
 
   // If not DWORD aligned or size is more than the threshold, call the library.
   // The libc version is likely to be faster for these cases. It can use the
   // address value and run time information about the CPU.
   if ((Align & 3) != 0 || !ConstantSize ||
X86SelectionDAGInfo::EmitTargetCodeForMemcpy(SelectionDAG &DAG, SDLoc dl,
   if (!AlwaysInline && (Align & 3) != 0)
     return SDValue();
 
   // If to a segment-relative address space, use the default lowering.
   if (DstPtrInfo.getAddrSpace() >= 256 ||
       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.
-  const X86RegisterInfo *TRI = static_cast<const X86RegisterInfo *>(
-      DAG.getSubtarget().getRegisterInfo());
-  if (TRI->hasBasePointer(DAG.getMachineFunction()) &&
-      TRI->getBaseRegister() == X86::ESI)
+  // If the base register might conflict with our physical registers, bail out.
+  unsigned ClobberSet[] = {X86::RCX, X86::RSI, X86::RDI,
+                           X86::ECX, X86::ESI, X86::EDI};
+  if (isBaseRegConflictPossible(DAG, ClobberSet))
     return SDValue();
 
   MVT AVT;
   if (Align & 1)
     AVT = MVT::i8;
   else if (Align & 2)
     AVT = MVT::i16;
   else if (Align & 4)

test/CodeGen/X86/mem-intrin-base-reg.ll

+; RUN: llc -mtriple=i686-windows -mattr=+sse2 < %s | FileCheck %s
+
+target datalayout = "e-m:w-p:32:32-i64:64-f80:32-n8:16:32-S32"
+target triple = "i686-pc-windows-msvc"
+
+; There is a conflict between lowering the X86 memory intrinsics and the "base"
+; register used to address stack locals.  See X86RegisterInfo::hasBaseRegister
+; for when this is necessary. Typically, we chose ESI for the base register,
+; which all of the X86 string instructions use.
+
+; The pattern of vector icmp and extractelement is used in these tests because
+; it forces creation of an aligned stack temporary. Perhaps such temporaries
+; shouldn't be aligned.
+
+declare void @escape_vla_and_icmp(i8*, i1 zeroext)
+declare void @llvm.memcpy.p0i8.p0i8.i32(i8* nocapture, i8* nocapture readonly, i32, i32, i1)
+declare void @llvm.memset.p0i8.i32(i8* nocapture, i8, i32, i32, i1)
+
+define i32 @memcpy_novla_vector(<4 x i32>* %vp0, i8* %a, i8* %b, i32 %n, i1 zeroext %cond) {
+  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %a, i8* %b, i32 128, i32 4, i1 false)
+  br i1 %cond, label %spill_vectors, label %no_vectors
+
+no_vectors:
+  ret i32 0
+
+spill_vectors:
+  %vp1 = getelementptr <4 x i32>* %vp0, i32 1
+  %v0 = load <4 x i32>* %vp0
+  %v1 = load <4 x i32>* %vp1
+  %vicmp = icmp slt <4 x i32> %v0, %v1
+  %icmp = extractelement <4 x i1> %vicmp, i32 0
+  call void @escape_vla_and_icmp(i8* null, i1 zeroext %icmp)
+  %r = extractelement <4 x i32> %v0, i32 0
+  ret i32 %r
+}
+
+; CHECK-LABEL: _memcpy_novla_vector:
+; CHECK: andl $-16, %esp
+; CHECK-DAG: movl $32, %ecx
+; CHECK-DAG: movl {{.*}}, %esi
+; CHECK-DAG: movl {{.*}}, %edi
+; CHECK: rep;movsl
+
+define i32 @memcpy_vla_vector(<4 x i32>* %vp0, i8* %a, i8* %b, i32 %n, i1 zeroext %cond) {
+  call void @llvm.memcpy.p0i8.p0i8.i32(i8* %a, i8* %b, i32 128, i32 4, i1 false)
+  br i1 %cond, label %spill_vectors, label %no_vectors
+
+no_vectors:
+  ret i32 0
+
+spill_vectors:
+  %vp1 = getelementptr <4 x i32>* %vp0, i32 1
+  %v0 = load <4 x i32>* %vp0
+  %v1 = load <4 x i32>* %vp1
+  %vicmp = icmp slt <4 x i32> %v0, %v1
+  %icmp = extractelement <4 x i1> %vicmp, i32 0
+  %vla = alloca i8, i32 %n
+  call void @escape_vla_and_icmp(i8* %vla, i1 zeroext %icmp)
+  %r = extractelement <4 x i32> %v0, i32 0
+  ret i32 %r
+}
+
+; CHECK-LABEL: _memcpy_vla_vector:
+; CHECK: andl $-16, %esp
+; CHECK: movl %esp, %esi
+; CHECK: movl $128, {{.*}}(%esp)
+; CHECK: calll _memcpy
+; CHECK: calll __chkstk
+
+; stosd doesn't clobber esi, so we can use it.
+
+define i32 @memset_vla_vector(<4 x i32>* %vp0, i8* %a, i32 %n, i1 zeroext %cond) {
+  call void @llvm.memset.p0i8.i32(i8* %a, i8 42, i32 128, i32 4, i1 false)
+  br i1 %cond, label %spill_vectors, label %no_vectors
+
+no_vectors:
+  ret i32 0
+
+spill_vectors:
+  %vp1 = getelementptr <4 x i32>* %vp0, i32 1
+  %v0 = load <4 x i32>* %vp0
+  %v1 = load <4 x i32>* %vp1
+  %vicmp = icmp slt <4 x i32> %v0, %v1
+  %icmp = extractelement <4 x i1> %vicmp, i32 0
+  %vla = alloca i8, i32 %n
+  call void @escape_vla_and_icmp(i8* %vla, i1 zeroext %icmp)
+  %r = extractelement <4 x i32> %v0, i32 0
+  ret i32 %r
+}
+
+; CHECK-LABEL: _memset_vla_vector:
+; CHECK: andl $-16, %esp
+; CHECK: movl %esp, %esi
+; CHECK-DAG: movl $707406378, %eax        # imm = 0x2A2A2A2A
+; CHECK-DAG: movl $32, %ecx
+; CHECK-DAG: movl {{.*}}, %edi
+; CHECK-NOT: movl {{.*}}, %esi
+; CHECK: rep;stosl
+
+; Add a test for memcmp if we ever add a special lowering for it.