[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 created this revision.
craig.topper added reviewers: MaskRay, rnk, RKSimon, spatel.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.

I might be wrong, but I think the main issue here is that we
earlied out of foldOffsetIntoAddress if Offset is 0. But if we
just found a symbolic displacement and AM.Disp became non-zero
earlier, we still need to validate that AM.Disp with the symbolic
displacement.

This passes the test case from D73606 <https://reviews.llvm.org/D73606>.


Repository:
  rG LLVM Github Monorepo

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,18 @@
 
 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)
+  int64_t Val = AM.Disp + Offset;
+
+  // 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 everyhing if Val is non-zero.
+  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 +1584,12 @@
   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 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: D73608.241063.patch
Type: text/x-patch
Size: 2360 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200129/98f7d101/attachment.bin>