[PATCH] D73606: [X86] matchAdd: don't fold large displacements

[Fangrui Song via Phabricator via llvm-commits]

MaskRay created this revision.
Herald added subscribers: llvm-commits, hiraditya.
Herald added a project: LLVM.

  X86DAGToDAGISel::matchAdd
    ...
    if (!matchAddressRecursively(N.getOperand(0), AM, Depth+1) &&
        !matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth+1))
      return false;
    ...
    // Try again after commuting the operands.
    if (!matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth+1) && /// sets displacement
        !matchAddressRecursively(Handle.getValue().getOperand(0), AM, Depth+1))   /// sets symbol
      return false;

For `leaq sym+disp(%rip),...`, `foldOffsetIntoAddress()` currently does
not know there is a symbolic displacement and would fold disp.

The produced `leaq symbol+disp(%rip), %rax` instruction is relocated by
an R_X86_64_PC32. If disp is large and symbol+disp-rip>=2**31, there
will be a relocation overflow.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D73606

Files:
  llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
  llvm/test/CodeGen/X86/fold-pcrel-add.ll


Index: llvm/test/CodeGen/X86/fold-pcrel-add.ll
===================================================================
--- /dev/null
+++ llvm/test/CodeGen/X86/fold-pcrel-add.ll
@@ -0,0 +1,41 @@
+; RUN: llc -mtriple=x86_64 -relocation-model=static < %s | FileCheck --check-prefixes=CHECK,STATIC %s
+; RUN: llc -mtriple=x86_64 -relocation-model=pic < %s | FileCheck --check-prefixes=CHECK,PIC %s
+; RUN: llc -mtriple=x86_64 -code-model=medium -relocation-model=static < %s | FileCheck --check-prefixes=CHECK,MSTATIC %s
+; RUN: llc -mtriple=x86_64 -code-model=medium -relocation-model=pic < %s | FileCheck --check-prefixes=CHECK,MPIC %s
+
+ at foo = dso_local global i32 0
+
+define dso_local i64 @zero() {
+; CHECK-LABEL: zero:
+; CHECK:       # %bb.0:
+; STATIC-NEXT:   movl $foo, %eax
+; STATIC-NEXT:   retq
+; PIC-NEXT:      leaq foo(%rip), %rax
+; PIC-NEXT:      retq
+; MSTATIC-NEXT:  movabsq $foo, %rax
+; MSTATIC-NEXT:  retq
+; MPIC-NEXT:     leaq _GLOBAL_OFFSET_TABLE_(%rip), %rcx
+; MPIC-NEXT:     movabsq $foo at GOTOFF, %rax
+; MPIC-NEXT:     addq %rcx, %rax
+entry:
+  ret i64 add (i64 ptrtoint (i32* @foo to i64), i64 0)
+}
+
+;; Check we don't fold a large offset into leaq, otherwise
+;; the large r_addend can easily cause a relocation overflow.
+define dso_local i64 @large() {
+; CHECK-LABEL: large:
+; CHECK:       # %bb.0:
+; STATIC-NEXT:   movl $1701208431, %eax
+; STATIC-NEXT:   leaq foo(%rax), %rax
+; PIC-NEXT:      leaq foo(%rip), %rax
+; PIC-NEXT:      addq $1701208431, %rax
+; MSTATIC-NEXT:  movabsq $foo, %rax
+; MSTATIC-NEXT:  addq $1701208431, %rax
+; MSTATIC-NEXT:  retq
+; MPIC-NEXT:     leaq _GLOBAL_OFFSET_TABLE_(%rip), %rax
+; MPIC-NEXT:     movabsq $foo at GOTOFF, %rcx
+; MPIC-NEXT:     leaq 1701208431(%rax,%rcx), %rax
+entry:
+  ret i64 add (i64 ptrtoint (i32* @foo to i64), i64 1701208431)
+}
Index: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
===================================================================
--- llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -1581,12 +1581,24 @@
   if (!matchAddressRecursively(N.getOperand(0), AM, Depth+1) &&
       !matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth+1))
     return false;
+
+  // Don't try commuting operands if the address is in the form of
+  // sym+disp(%rip). foldOffsetIntoAddress() currently does not know there is a
+  // symbolic displacement and would fold disp. If disp is just a bit smaller
+  // than 2**31, it can easily cause a relocation overflow.
+  bool NoCommute = false;
+  if (AM.isRIPRelative() && AM.hasSymbolicDisplacement())
+    if (ConstantSDNode *Cst =
+            dyn_cast<ConstantSDNode>(Handle.getValue().getOperand(1)))
+      NoCommute = Cst->getSExtValue() != 0;
+
   AM = Backup;
-
-  // Try again after commuting the operands.
-  if (!matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth+1) &&
-      !matchAddressRecursively(Handle.getValue().getOperand(0), AM, Depth+1))
-    return false;
+  if (!NoCommute) {
+    // Try again after commuting the operands.
+    if (!matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth + 1) &&
+        !matchAddressRecursively(Handle.getValue().getOperand(0), AM, Depth + 1))
+      return false;
+  }
   AM = Backup;
 
   // If we couldn't fold both operands into the address at the same time,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D73606.241051.patch
Type: text/x-patch
Size: 3336 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200129/3667bfa3/attachment.bin>