[PATCH] D73608: [X86] Alternative implementation of D73606 matchAdd: don't fold a large offset into a %rip relative address

[Craig Topper via Phabricator via llvm-commits]

craig.topper updated this revision to Diff 241317.
craig.topper edited the summary of this revision.
craig.topper added a comment.

Address review comments


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D73608/new/

https://reviews.llvm.org/D73608

Files:
  llvm/lib/Target/X86/X86ISelDAGToDAG.cpp


Index: llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
===================================================================
--- llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
+++ llvm/lib/Target/X86/X86ISelDAGToDAG.cpp
@@ -1409,15 +1409,17 @@
 
 bool X86DAGToDAGISel::foldOffsetIntoAddress(uint64_t Offset,
                                             X86ISelAddressMode &AM) {
-  // If there's no offset to fold, we don't need to do any work.
-  if (Offset == 0)
+  // If the final displacement is 0, we don't need to do any work. We may have
+  // already matched a displacement and the caller just added the symbolic
+  // displacement with an offset of 0. So recheck everything if Val is non-zero.
+  int64_t Val = AM.Disp + Offset;
+  if (Val == 0)
     return false;
 
   // Cannot combine ExternalSymbol displacements with integer offsets.
   if (AM.ES || AM.MCSym)
     return true;
 
-  int64_t Val = AM.Disp + Offset;
   CodeModel::Model M = TM.getCodeModel();
   if (Subtarget->is64Bit()) {
     if (!X86::isOffsetSuitableForCodeModel(Val, M,
@@ -1581,24 +1583,13 @@
   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 NoCommutate = false;
-  if (AM.isRIPRelative() && AM.hasSymbolicDisplacement())
-    if (ConstantSDNode *Cst =
-            dyn_cast<ConstantSDNode>(Handle.getValue().getOperand(1)))
-      NoCommutate = Cst->getSExtValue() != 0;
-
   AM = Backup;
-  if (!NoCommutate) {
-    // Try again after commutating the operands.
-    if (!matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth + 1) &&
-        !matchAddressRecursively(Handle.getValue().getOperand(0), AM, Depth + 1))
-      return false;
-  }
+
+  // Try again after commutating 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: D73608.241317.patch
Type: text/x-patch
Size: 2397 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200130/89019b22/attachment.bin>