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

[craig.topper]

Summary

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 fold-add-pcrel.ll.

[MaskRay]

Thanks for the patch! Nice clean-up! I only get some nits.

This passes the test case from D73606.

Be a bit more explicit and just mention fold-add-pcrel.s

I am still wondering whether we can simplify %rip relative offset folding. Whether FIXME like below can be fixed:

// FIXME: JumpTable and ExternalSymbol address currently don't like
// displacements.  It isn't very important, but this should be fixed for
// consistency.
if (!(AM.ES || AM.MCSym) && AM.JT != -1)
  return true;

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp

1416

everying -> everything

int64_t Val = AM.Disp + Offset; can be placed immediately below the comment.

1588

commuting -> commutating

1589

clang-format will prefer Depth + 1

[craig.topper]

Address review comments

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp

static bool isDispSafeForFrameIndex(int64_t Val) {
   // 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.
   return isInt<31>(Val);
 }
 
 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,
                                            AM.hasSymbolicDisplacement()))
       return true;
     // 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 &&
bool X86DAGToDAGISel::matchAdd(SDValue &N, X86ISelAddressMode &AM,
   // Add an artificial use to this node so that we can keep track of
   // it if it gets CSE'd with a different node.
   HandleSDNode Handle(N);
 
   X86ISelAddressMode Backup = AM;
   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;
-  }
+
+  if (!matchAddressRecursively(Handle.getValue().getOperand(1), AM,
+                               Depth + 1) &&
   AM = Backup;
 
   // If we couldn't fold both operands into the address at the same time,
   // see if we can just put each operand into a register and fold at least
   // the add.
   if (AM.BaseType == X86ISelAddressMode::RegBase &&
       !AM.Base_Reg.getNode() &&
       !AM.IndexReg.getNode()) {