[X86] Fix isOffsetSuitableForCodeModel

[MaskRay]

Summary

If hasSymbolicDisplacement is false, there may still be a symbolic
displacement due to the order operands are matched.

X86DAGToDAGISel::matchAdd
  ...
  // 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;

This can create a leaq symbol+disp(%rip), %rax instruction relocated by
R_X86_64_PC32. If symbol+disp-rip>=2**31 there will be a relocation overflow.

[merge_guards_bot]

Unit tests: pass. 62278 tests passed, 0 failed and 827 were skipped.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[craig.topper]

Instead of a new parameter is it possible to work something into the AddressingMode struct so that it gets restored when we restore from a backup AM?

Do you have a test case?

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp

213

This name doesn't match what's used in the definition of the function.

1594

Do we need to resync HasSymbolicDisp from the restored AM if one of the matchAddressRecursively calls failed the match but had modified the flag?

2098

Why is it called Force here?

[craig.topper]

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp

1415

Without the test case, I can't be sure. But is the bug really just here where we don't check anything if the Offset is 0. But we still need to do the check if AM.Disp is already non-zero?

[MaskRay]

In D73601#1846047, @craig.topper wrote:

Instead of a new parameter is it possible to work something into the AddressingMode struct so that it gets restored when we restore from a backup AM?

Do you have a test case?

The bug was somewhat urgent and was breaking our release. @craig.topper So sorry, I went ahead and committed D73606.

I made more description there.

llvm/lib/Target/X86/X86ISelDAGToDAG.cpp

public:
     void PostprocessISelDAG() override;
 
 // Include the pieces autogenerated from the target description.
 #include "X86GenDAGISel.inc"
 
   private:
     void Select(SDNode *N) override;
 
-    bool foldOffsetIntoAddress(uint64_t Offset, X86ISelAddressMode &AM);
+    bool foldOffsetIntoAddress(uint64_t Offset, X86ISelAddressMode &AM,
+                               bool ForceSymbolicDisp = false);
     bool matchLoadInAddress(LoadSDNode *N, X86ISelAddressMode &AM);
-    bool matchWrapper(SDValue N, X86ISelAddressMode &AM);
+    bool matchWrapper(SDValue N, X86ISelAddressMode &AM, bool &HasSymbolicDisp);
     bool matchAddress(SDValue N, X86ISelAddressMode &AM);
     bool matchVectorAddress(SDValue N, X86ISelAddressMode &AM);
     bool matchAdd(SDValue &N, X86ISelAddressMode &AM, unsigned Depth);
     bool matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
-                                 unsigned Depth);
+                                 unsigned Depth, bool &HasSymbolicDisp);
     bool matchAddressBase(SDValue N, X86ISelAddressMode &AM);
     bool selectAddr(SDNode *Parent, SDValue N, SDValue &Base,
                     SDValue &Scale, SDValue &Index, SDValue &Disp,
                     SDValue &Segment);
     bool selectVectorAddr(SDNode *Parent, SDValue N, SDValue &Base,
                           SDValue &Scale, SDValue &Index, SDValue &Disp,
                           SDValue &Segment);
     bool selectMOV64Imm32(SDValue N, SDValue &Imm);
static bool isDispSafeForFrameIndex(int64_t Val) {
   // will overflow the displacement field. Assuming that the frame index
   // 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) {
+                                            X86ISelAddressMode &AM,
+                                            bool HasSymbolicDisp) {
   // If there's no offset to fold, we don't need to do any work.
   if (Offset == 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()))
+    if (!X86::isOffsetSuitableForCodeModel(
+            Val, M, HasSymbolicDisp || 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 &&
         !isDispSafeForFrameIndex(Val))
       return true;
   }
   AM.Disp = Val;
   return false;
-
 }
 
 bool X86DAGToDAGISel::matchLoadInAddress(LoadSDNode *N, X86ISelAddressMode &AM){
   SDValue Address = N->getOperand(1);
 
   // load gs:0 -> GS segment register.
   // load fs:0 -> FS segment register.
   //
if (C->getSExtValue() == 0 && AM.Segment.getNode() == nullptr &&
       }
 
   return true;
 }
 
 /// Try to match X86ISD::Wrapper and X86ISD::WrapperRIP nodes into an addressing
 /// mode. These wrap things that will resolve down into a symbol reference.
 /// If no match is possible, this returns true, otherwise it returns false.
-bool X86DAGToDAGISel::matchWrapper(SDValue N, X86ISelAddressMode &AM) {
+bool X86DAGToDAGISel::matchWrapper(SDValue N, X86ISelAddressMode &AM,
+                                   bool &HasSymbolicDisp) {
   // If the addressing mode already has a symbol as the displacement, we can
   // never match another symbol.
   if (AM.hasSymbolicDisplacement())
     return true;
 
   bool IsRIPRelTLS = false;
   bool IsRIPRel = N.getOpcode() == X86ISD::WrapperRIP;
   if (IsRIPRel) {
if (foldOffsetIntoAddress(Offset, AM)) {
     AM = Backup;
     return true;
   }
 
   if (IsRIPRel)
     AM.setBaseReg(CurDAG->getRegister(X86::RIP, MVT::i64));
 
   // Commit the changes now that we know this fold is safe.
+  HasSymbolicDisp = true;
   return false;
 }
 
 /// Add the specified node to the specified addressing mode, returning true if
 /// it cannot be done. This just pattern matches for the addressing mode.
 bool X86DAGToDAGISel::matchAddress(SDValue N, X86ISelAddressMode &AM) {
-  if (matchAddressRecursively(N, AM, 0))
+  bool HasSymbolicDisp = false;
+  if (matchAddressRecursively(N, AM, 0, HasSymbolicDisp))
     return true;
 
   // Post-processing: Convert lea(,%reg,2) to lea(%reg,%reg), which has
   // a smaller encoding and avoids a scaled-index.
   if (AM.Scale == 2 &&
       AM.BaseType == X86ISelAddressMode::RegBase &&
       AM.Base_Reg.getNode() == nullptr) {
     AM.Base_Reg = AM.IndexReg;

 }
 
 bool X86DAGToDAGISel::matchAdd(SDValue &N, X86ISelAddressMode &AM,
                                unsigned Depth) {
   // 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);
 
+  bool HasSymbolicDisp = AM.hasSymbolicDisplacement();
   X86ISelAddressMode Backup = AM;
-  if (!matchAddressRecursively(N.getOperand(0), AM, Depth+1) &&
-      !matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth+1))
+  if (!matchAddressRecursively(N.getOperand(0), AM, Depth + 1, HasSymbolicDisp) &&
+      !matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth + 1,
+                               HasSymbolicDisp))
     return false;
   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))
+  if (!matchAddressRecursively(Handle.getValue().getOperand(1), AM, Depth + 1,
+                               HasSymbolicDisp) &&
+      !matchAddressRecursively(Handle.getValue().getOperand(0), AM, Depth + 1,
+                               HasSymbolicDisp))
     return false;
   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() &&
static bool foldMaskedShiftToBEXTR(SelectionDAG &DAG, SDValue N,
   DAG.RemoveDeadNode(N.getNode());
 
   AM.Scale = 1 << AMShiftAmt;
   AM.IndexReg = NewAnd;
   return false;
 }
 
 bool X86DAGToDAGISel::matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
-                                              unsigned Depth) {
+                                              unsigned Depth,
+                                              bool &HasSymbolicDisp) {
   SDLoc dl(N);
   LLVM_DEBUG({
     dbgs() << "MatchAddress: ";
     AM.dump(CurDAG);
   });
   // Limit recursion.
   if (Depth > 5)
     return matchAddressBase(N, AM);
if (!AM.hasSymbolicDisplacement() && AM.Disp == 0)
         // Use the symbol and don't prefix it.
         AM.MCSym = ESNode->getMCSymbol();
         return false;
       }
     break;
   }
   case ISD::Constant: {
     uint64_t Val = cast<ConstantSDNode>(N)->getSExtValue();
-    if (!foldOffsetIntoAddress(Val, AM))
+    if (!foldOffsetIntoAddress(Val, AM, HasSymbolicDisp))
       return false;
     break;
   }
 
   case X86ISD::Wrapper:
   case X86ISD::WrapperRIP:
-    if (!matchWrapper(N, AM))
+    if (!matchWrapper(N, AM, HasSymbolicDisp))
       return false;
     break;
 
   case ISD::LOAD:
     if (!matchLoadInAddress(cast<LoadSDNode>(N), AM))
       return false;
     break;
 
case ISD::SUB: {
     // it costs an additional mov if the index register has other uses.
 
     // 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);
 
     // Test if the LHS of the sub can be folded.
     X86ISelAddressMode Backup = AM;
-    if (matchAddressRecursively(N.getOperand(0), AM, Depth+1)) {
+    bool ForceSymbolicDisp = false;
+    if (matchAddressRecursively(N.getOperand(0), AM, Depth + 1,
+                                ForceSymbolicDisp)) {
       N = Handle.getValue();
       AM = Backup;
       break;
     }
     N = Handle.getValue();
     // Test if the index field is free for use.
     if (AM.IndexReg.getNode() || AM.isRIPRelative()) {
       AM = Backup;
bool X86DAGToDAGISel::matchVectorAddress(SDValue N, X86ISelAddressMode &AM) {
   // TODO: Support other operations.
   switch (N.getOpcode()) {
   case ISD::Constant: {
     uint64_t Val = cast<ConstantSDNode>(N)->getSExtValue();
     if (!foldOffsetIntoAddress(Val, AM))
       return false;
     break;
   }
-  case X86ISD::Wrapper:
-    if (!matchWrapper(N, AM))
+  case X86ISD::Wrapper: {
+    bool HasSymbolicDisp = false;
+    if (!matchWrapper(N, AM, HasSymbolicDisp))
       return false;
     break;
   }
+  }
 
   return matchAddressBase(N, AM);
 }
 
 bool X86DAGToDAGISel::selectVectorAddr(SDNode *Parent, SDValue N, SDValue &Base,
                                        SDValue &Scale, SDValue &Index,
                                        SDValue &Disp, SDValue &Segment) {
   X86ISelAddressMode AM;