[X86] Don't fold very large offsets into addr displacements during ISel

[wesleywiser]

Doing so can cause the resulting displacement after frame layout to become inexpressible (or cause over/underflow currently during frame layout).

Fixes the error reported in #101840 (comment).

[llvmbot]

@llvm/pr-subscribers-backend-x86

Author: Wesley Wiser (wesleywiser)

Changes

Doing so can cause the resulting displacement after frame layout to become inexpressible (or cause over/underflow currently during frame layout).

Fixes the error reported in #101840 (comment).

---

Full diff: https://github.com/llvm/llvm-project/pull/121678.diff

3 Files Affected:

• (modified) llvm/lib/Target/X86/X86ISelDAGToDAG.cpp (+9-6)
• (added) llvm/test/CodeGen/X86/dag-large-offset.ll (+36)
• (modified) llvm/test/CodeGen/X86/xor-lea.ll (+2-1)

diff --git a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
index 9b340a778b36ad..2549bae4ae2efd 100644
--- a/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ b/llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -1800,10 +1800,10 @@ void X86DAGToDAGISel::emitFunctionEntryCode() {
     emitSpecialCodeForMain();
 }
 
-static bool isDispSafeForFrameIndex(int64_t Val) {
-  // On 64-bit platforms, we can run into an issue where a frame index
+static bool isDispSafeForFrameIndexOrRegBase(int64_t Val) {
+  // We can run into an issue where a frame index or a register base
   // includes a displacement that, when added to the explicit displacement,
-  // will overflow the displacement field. Assuming that the frame index
+  // will overflow the displacement field. Assuming that the
   // displacement fits into a 31-bit integer  (which is only slightly more
   // aggressive than the current fundamental assumption that it fits into
   // a 32-bit integer), a 31-bit disp should always be safe.
@@ -1831,7 +1831,7 @@ bool X86DAGToDAGISel::foldOffsetIntoAddress(uint64_t Offset,
     // In addition to the checks required for a register base, check that
     // we do not try to use an unsafe Disp with a frame index.
     if (AM.BaseType == X86ISelAddressMode::FrameIndexBase &&
-        !isDispSafeForFrameIndex(Val))
+        !isDispSafeForFrameIndexOrRegBase(Val))
       return true;
     // In ILP32 (x32) mode, pointers are 32 bits and need to be zero-extended to
     // 64 bits. Instructions with 32-bit register addresses perform this zero
@@ -1852,7 +1852,10 @@ bool X86DAGToDAGISel::foldOffsetIntoAddress(uint64_t Offset,
     if (Subtarget->isTarget64BitILP32() && !isUInt<31>(Val) &&
         !AM.hasBaseOrIndexReg())
       return true;
-  }
+  } else if (AM.hasBaseOrIndexReg() && !isDispSafeForFrameIndexOrRegBase(Val))
+    // For 32-bit X86, make sure the displacement still isn't close to the
+    // expressible limit.
+    return true;
   AM.Disp = Val;
   return false;
 }
@@ -2553,7 +2556,7 @@ bool X86DAGToDAGISel::matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
   case ISD::FrameIndex:
     if (AM.BaseType == X86ISelAddressMode::RegBase &&
         AM.Base_Reg.getNode() == nullptr &&
-        (!Subtarget->is64Bit() || isDispSafeForFrameIndex(AM.Disp))) {
+        (!Subtarget->is64Bit() || isDispSafeForFrameIndexOrRegBase(AM.Disp))) {
       AM.BaseType = X86ISelAddressMode::FrameIndexBase;
       AM.Base_FrameIndex = cast<FrameIndexSDNode>(N)->getIndex();
       return false;
diff --git a/llvm/test/CodeGen/X86/dag-large-offset.ll b/llvm/test/CodeGen/X86/dag-large-offset.ll
new file mode 100644
index 00000000000000..93d49a34fc834a
--- /dev/null
+++ b/llvm/test/CodeGen/X86/dag-large-offset.ll
@@ -0,0 +1,36 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 5
+; RUN: llc < %s -mtriple=i386-pc-windows-msvc | FileCheck %s
+
+define dso_local noundef i32 @foo(i1 %b) local_unnamed_addr #0 {
+; CHECK-LABEL: foo:
+; CHECK:       # %bb.0: # %entry
+; CHECK-NEXT:    pushl %ebp
+; CHECK-NEXT:    movl %esp, %ebp
+; CHECK-NEXT:    pushl %esi
+; CHECK-NEXT:    subl $8, %esp
+; CHECK-NEXT:    movl ___security_cookie, %eax
+; CHECK-NEXT:    xorl %ebp, %eax
+; CHECK-NEXT:    movl %eax, -8(%ebp)
+; CHECK-NEXT:    movl $-2147483647, %eax # imm = 0x80000001
+; CHECK-NEXT:    addl %ebp, %eax
+; CHECK-NEXT:    addl $-9, %eax
+; CHECK-NEXT:    xorl %esi, %esi
+; CHECK-NEXT:    testb $1, 8(%ebp)
+; CHECK-NEXT:    cmovnel %eax, %esi
+; CHECK-NEXT:    movl -8(%ebp), %ecx
+; CHECK-NEXT:    xorl %ebp, %ecx
+; CHECK-NEXT:    calll @__security_check_cookie@4
+; CHECK-NEXT:    movl %esi, %eax
+; CHECK-NEXT:    addl $8, %esp
+; CHECK-NEXT:    popl %esi
+; CHECK-NEXT:    popl %ebp
+; CHECK-NEXT:    retl
+entry:
+  %a = alloca i8, align 1
+  %0 = ptrtoint ptr %a to i32
+  %sub = add i32 %0, -2147483647
+  %retval.0 = select i1 %b, i32 %sub, i32 0
+  ret i32 %retval.0
+}
+
+attributes #0 = { mustprogress nofree norecurse nosync nounwind sspreq willreturn memory(none) "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="pentium4" "target-features"="+cmov,+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
diff --git a/llvm/test/CodeGen/X86/xor-lea.ll b/llvm/test/CodeGen/X86/xor-lea.ll
index 10e9525a2706a3..d50752e48d293f 100644
--- a/llvm/test/CodeGen/X86/xor-lea.ll
+++ b/llvm/test/CodeGen/X86/xor-lea.ll
@@ -327,7 +327,8 @@ define i32 @xor_shl_sminval_i32(i32 %x) {
 ; X86-LABEL: xor_shl_sminval_i32:
 ; X86:       # %bb.0:
 ; X86-NEXT:    movl {{[0-9]+}}(%esp), %eax
-; X86-NEXT:    leal -2147483648(,%eax,8), %eax
+; X86-NEXT:    movl $-2147483648, %ecx # imm = 0x80000000
+; X86-NEXT:    leal (%ecx,%eax,8), %eax
 ; X86-NEXT:    retl
 ;
 ; X64-LABEL: xor_shl_sminval_i32:

[wesleywiser]

Hi @RKSimon and @phoebewang, do either of you have time to review this patch? Thanks!

[phoebewang]

Change this to isDispSafeForFrameIndexOrRegBase too?

[RKSimon]

Do we know why its matching for unsigned here?

[wesleywiser]

Changed. The use of unsigned here (and the cast I've updated this to) is because of 8d52097.

[phoebewang]

LGTM.

[wesleywiser]

Thanks, I don't have commit rights so if someone would take care of merging for me, that would be appreciated!