[inlineasm] Propagate operand constraints to the backend

[sdardis]

Summary

When SelectionDAGISel transforms a node representing an inline asm
block, memory constraint information is not preserved. This can cause
constraints to be broken when a memory offset is of the form:

offset + frame index

when the frame is resolved.

By propagating the constraints all the way to the backend, targets can
enforce memory operands of inline assembly to conform to their constraints.

For MIPSR6, some instructions had their offsets reduced to 9 bits from
16 bits such as ll/sc. This becomes problematic when using inline assembly
to perform atomic operations, as an offset can generated that is too big to
encode in the instruction.

[dsanders]

This patch is going in the right direction but I think there's a couple extra things we ought to do to round this off:

• Stop MachineInstr::print() from printing register constraints when the flag do not hold a register constraint.
• Make MachineInstr::print() print the memory constraint when the flags have one.
• Add assertions to InlineAsm::getFlagWordForRegClass() and Inline::getFlagWordForMem() to make sure we cannot put the wrong kind of constraint in the relevant flag bits.

I also think the summary needs to mention that frameindex nodes introduce the out of range offsets that are causing the problem. Without the frameindex nodes we would have dealt with the out-of-range offsets during isel.

lib/Target/Mips/MipsSERegisterInfo.cpp

66–127

I don't mind if we do this in a separate patch but we need to cover the various versions of the LL/SC instructions in this switch statement too.

68

Scoping nit: Please move this into the 'case Mips::INLINE_ASM:', adding the necessary braces to allow this.

85

This value depends on the ISA. It should be 9-bit for MIPS32R6/MIPS64R6 and 16-bit for earlier ISA's.

213–214

This is over 80 columns

test/CodeGen/Mips/inlineasm-constraint_ZC_2.ll

2–3

Please add a pre-R6 test to prove that 9-16 bit offsets are still accepted by the inline assembly for this case.

15–18

For the R6 case, could you check for the addiu too?

[sdardis]

Updated as per review comments, retitled and new summary.

[dsanders]

LGTM with the pre-R6 test and the addiu check

test/CodeGen/Mips/inlineasm-constraint_ZC_2.ll

2–3

This hasn't been done

15–18

This hasn't been done.

[sdardis]

Thanks, committed rL275786.

include/llvm/IR/InlineAsm.h

public:
   };
 
   static unsigned getFlagWord(unsigned Kind, unsigned NumOps) {
     assert(((NumOps << 3) & ~0xffff) == 0 && "Too many inline asm operands!");
     assert(Kind >= Kind_RegUse && Kind <= Kind_Mem && "Invalid Kind");
     return Kind | (NumOps << 3);
   }
 
+  static bool isRegDefKind(unsigned Flag){ return getKind(Flag) == Kind_RegDef;}
+  static bool isImmKind(unsigned Flag) { return getKind(Flag) == Kind_Imm; }
+  static bool isMemKind(unsigned Flag) { return getKind(Flag) == Kind_Mem; }
+  static bool isRegDefEarlyClobberKind(unsigned Flag) {
+    return getKind(Flag) == Kind_RegDefEarlyClobber;
+  }
+  static bool isClobberKind(unsigned Flag) {
+    return getKind(Flag) == Kind_Clobber;
+  }
+
   /// getFlagWordForMatchingOp - Augment an existing flag word returned by
   /// getFlagWord with information indicating that this input operand is tied
   /// to a previous output operand.
   static unsigned getFlagWordForMatchingOp(unsigned InputFlag,
                                            unsigned MatchedOperandNo) {
     assert(MatchedOperandNo <= 0x7fff && "Too big matched operand");
     assert((InputFlag & ~0xffff) == 0 && "High bits already contain data");
     return InputFlag | Flag_MatchingOperand | (MatchedOperandNo << 16);
   }
 
   /// getFlagWordForRegClass - Augment an existing flag word returned by
   /// getFlagWord with the required register class for the following register
   /// operands.
   /// A tied use operand cannot have a register class, use the register class
   /// from the def operand instead.
   static unsigned getFlagWordForRegClass(unsigned InputFlag, unsigned RC) {
     // Store RC + 1, reserve the value 0 to mean 'no register class'.
     ++RC;
+    assert(!isImmKind(InputFlag) && "Immediates cannot have a register class");
+    assert(!isMemKind(InputFlag) && "Memory operand cannot have a register class");
     assert(RC <= 0x7fff && "Too large register class ID");
     assert((InputFlag & ~0xffff) == 0 && "High bits already contain data");
     return InputFlag | (RC << 16);
   }
 
   /// Augment an existing flag word returned by getFlagWord with the constraint
   /// code for a memory constraint.
   static unsigned getFlagWordForMem(unsigned InputFlag, unsigned Constraint) {
+    assert(isMemKind(InputFlag) && "InputFlag is not a memory constraint!");
     assert(Constraint <= 0x7fff && "Too large a memory constraint ID");
     assert(Constraint <= Constraints_Max && "Unknown constraint ID");
     assert((InputFlag & ~0xffff) == 0 && "High bits already contain data");
     return InputFlag | (Constraint << Constraints_ShiftAmount);
   }
 
   static unsigned convertMemFlagWordToMatchingFlagWord(unsigned InputFlag) {
     assert(isMemKind(InputFlag));
     return InputFlag & ~(0x7fff << Constraints_ShiftAmount);
   }
 
   static unsigned getKind(unsigned Flags) {
     return Flags & 7;
   }
 
-  static bool isRegDefKind(unsigned Flag){ return getKind(Flag) == Kind_RegDef;}
-  static bool isImmKind(unsigned Flag) { return getKind(Flag) == Kind_Imm; }
-  static bool isMemKind(unsigned Flag) { return getKind(Flag) == Kind_Mem; }
-  static bool isRegDefEarlyClobberKind(unsigned Flag) {
-    return getKind(Flag) == Kind_RegDefEarlyClobber;
-  }
-  static bool isClobberKind(unsigned Flag) {
-    return getKind(Flag) == Kind_Clobber;
-  }
-
   static unsigned getMemoryConstraintID(unsigned Flag) {
     assert(isMemKind(Flag));
     return (Flag >> Constraints_ShiftAmount) & 0x7fff;
   }
 
   /// getNumOperandRegisters - Extract the number of registers field from the
   /// inline asm operand flag.
   static unsigned getNumOperandRegisters(unsigned Flag) {

lib/CodeGen/MachineInstr.cpp

MachineInstr::getRegClassConstraint(unsigned OpIdx,
 
   // Inline asm stores register class constraints in the flag word.
   int FlagIdx = findInlineAsmFlagIdx(OpIdx);
   if (FlagIdx < 0)
     return nullptr;
 
   unsigned Flag = getOperand(FlagIdx).getImm();
   unsigned RCID;
-  if (InlineAsm::hasRegClassConstraint(Flag, RCID))
+  if ((InlineAsm::getKind(Flag) == InlineAsm::Kind_RegUse ||
+       InlineAsm::getKind(Flag) == InlineAsm::Kind_RegDef ||
+       InlineAsm::getKind(Flag) == InlineAsm::Kind_RegDefEarlyClobber) &&
+      InlineAsm::hasRegClassConstraint(Flag, RCID))
     return TRI->getRegClass(RCID);
 
   // Assume that all registers in a memory operand are pointers.
   if (InlineAsm::getKind(Flag) == InlineAsm::Kind_Mem)
     return TRI->getPointerRegClass(MF);
 
   return nullptr;
 }
if (isDebugValue() && MO.isMetadata()) {
       case InlineAsm::Kind_RegDefEarlyClobber: OS << ":[regdef-ec"; break;
       case InlineAsm::Kind_Clobber:            OS << ":[clobber"; break;
       case InlineAsm::Kind_Imm:                OS << ":[imm"; break;
       case InlineAsm::Kind_Mem:                OS << ":[mem"; break;
       default: OS << ":[??" << InlineAsm::getKind(Flag); break;
       }
 
       unsigned RCID = 0;
-      if (InlineAsm::hasRegClassConstraint(Flag, RCID)) {
+      if (!InlineAsm::isImmKind(Flag) && !InlineAsm::isMemKind(Flag) &&
+          InlineAsm::hasRegClassConstraint(Flag, RCID)) {
         if (TRI) {
           OS << ':' << TRI->getRegClassName(TRI->getRegClass(RCID));
         } else
           OS << ":RC" << RCID;
       }
 
+      if (InlineAsm::isMemKind(Flag)) {
+        unsigned MCID = InlineAsm::getMemoryConstraintID(Flag);
+        switch (MCID) {
+        case InlineAsm::Constraint_es: OS << ":es"; break;
+        case InlineAsm::Constraint_i:  OS << ":i"; break;
+        case InlineAsm::Constraint_m:  OS << ":m"; break;
+        case InlineAsm::Constraint_o:  OS << ":o"; break;
+        case InlineAsm::Constraint_v:  OS << ":v"; break;
+        case InlineAsm::Constraint_Q:  OS << ":Q"; break;
+        case InlineAsm::Constraint_R:  OS << ":R"; break;
+        case InlineAsm::Constraint_S:  OS << ":S"; break;
+        case InlineAsm::Constraint_T:  OS << ":T"; break;
+        case InlineAsm::Constraint_Um: OS << ":Um"; break;
+        case InlineAsm::Constraint_Un: OS << ":Un"; break;
+        case InlineAsm::Constraint_Uq: OS << ":Uq"; break;
+        case InlineAsm::Constraint_Us: OS << ":Us"; break;
+        case InlineAsm::Constraint_Ut: OS << ":Ut"; break;
+        case InlineAsm::Constraint_Uv: OS << ":Uv"; break;
+        case InlineAsm::Constraint_Uy: OS << ":Uy"; break;
+        case InlineAsm::Constraint_X:  OS << ":X"; break;
+        case InlineAsm::Constraint_Z:  OS << ":Z"; break;
+        case InlineAsm::Constraint_ZC: OS << ":ZC"; break;
+        case InlineAsm::Constraint_Zy: OS << ":Zy"; break;
+        default: OS << ":?"; break;
+        }
+      }
+
       unsigned TiedTo = 0;
       if (InlineAsm::isUseOperandTiedToDef(Flag, TiedTo))
         OS << " tiedto:$" << TiedTo;
 
       OS << ']';
 
       // Compute the index of the next operand descriptor.
       AsmDescOp += 1 + InlineAsm::getNumOperandRegisters(Flag);

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp

if (!InlineAsm::isMemKind(Flags)) {
         for (; TiedToOperand; --TiedToOperand) {
           CurOp += InlineAsm::getNumOperandRegisters(Flags)+1;
           Flags = cast<ConstantSDNode>(InOps[CurOp])->getZExtValue();
         }
       }
 
       // Otherwise, this is a memory operand.  Ask the target to select it.
       std::vector<SDValue> SelOps;
-      if (SelectInlineAsmMemoryOperand(InOps[i+1],
-                                       InlineAsm::getMemoryConstraintID(Flags),
-                                       SelOps))
+      unsigned ConstraintID = InlineAsm::getMemoryConstraintID(Flags);
+      if (SelectInlineAsmMemoryOperand(InOps[i+1], ConstraintID, SelOps))
         report_fatal_error("Could not match memory address.  Inline asm"
                            " failure!");
 
       // Add this to the output node.
       unsigned NewFlags =
         InlineAsm::getFlagWord(InlineAsm::Kind_Mem, SelOps.size());
+      NewFlags = InlineAsm::getFlagWordForMem(NewFlags, ConstraintID);
       Ops.push_back(CurDAG->getTargetConstant(NewFlags, DL, MVT::i32));
       Ops.insert(Ops.end(), SelOps.begin(), SelOps.end());
       i += 2;
     }
   }
 
   // Add the glue input back if present.
   if (e != InOps.size())

lib/Target/Mips/MipsSERegisterInfo.cpp

 MipsSERegisterInfo::intRegClass(unsigned Size) const {
   if (Size == 4)
     return &Mips::GPR32RegClass;
 
   assert(Size == 8);
   return &Mips::GPR64RegClass;
 }
 
-/// Get the size of the offset supported by the given load/store.
+/// Get the size of the offset supported by the given load/store/inline asm.
 /// The result includes the effects of any scale factors applied to the
 /// instruction immediate.
-static inline unsigned getLoadStoreOffsetSizeInBits(const unsigned Opcode) {
+static inline unsigned getLoadStoreOffsetSizeInBits(const unsigned Opcode,
+                                                    MachineOperand MO) {
   switch (Opcode) {
   case Mips::LD_B:
   case Mips::ST_B:
     return 10;
   case Mips::LD_H:
   case Mips::ST_H:
     return 10 + 1 /* scale factor */;
   case Mips::LD_W:
   case Mips::ST_W:
     return 10 + 2 /* scale factor */;
   case Mips::LD_D:
   case Mips::ST_D:
     return 10 + 3 /* scale factor */;
+  case Mips::LL:
+  case Mips::LL64:
+  case Mips::LLD:
+  case Mips::LLE:
+  case Mips::SC:
+  case Mips::SC64:
+  case Mips::SCD:
+  case Mips::SCE:
+    return 16;
+  case Mips::LLE_MM:
+  case Mips::LLE_MMR6:
+  case Mips::LL_MM:
+  case Mips::SCE_MM:
+  case Mips::SCE_MMR6:
+  case Mips::SC_MM:
+    return 12;
+  case Mips::LL64_R6:
+  case Mips::LL_R6:
+  case Mips::LLD_R6:
+  case Mips::SC64_R6:
+  case Mips::SCD_R6:
+  case Mips::SC_R6:
+    return 9;
+  case Mips::INLINEASM: {
+    unsigned ConstraintID = InlineAsm::getMemoryConstraintID(MO.getImm());
+    switch (ConstraintID) {
+    case InlineAsm::Constraint_ZC: {
+      const MipsSubtarget &Subtarget = MO.getParent()
+                                           ->getParent()
+                                           ->getParent()
+                                           ->getSubtarget<MipsSubtarget>();
+      if (Subtarget.inMicroMipsMode())
+        return 12;
+
+      if (Subtarget.hasMips32r6())
+        return 9;
+
+      return 16;
+    }
+    default:
+      return 16;
+    }
+  }
   default:
     return 16;
   }
 }
 
 /// Get the scale factor applied to the immediate in the given load/store.
 static inline unsigned getLoadStoreOffsetAlign(const unsigned Opcode) {
   switch (Opcode) {
   case Mips::LD_H:
   case Mips::ST_H:
     return 2;
   case Mips::LD_W:
void MipsSERegisterInfo::eliminateFI(MachineBasicBlock::iterator II,
   Offset += MI.getOperand(OpNo + 1).getImm();
 
   DEBUG(errs() << "Offset     : " << Offset << "\n" << "<--------->\n");
 
   if (!MI.isDebugValue()) {
     // Make sure Offset fits within the field available.
     // For MSA instructions, this is a 10-bit signed immediate (scaled by
     // element size), otherwise it is a 16-bit signed immediate.
-    unsigned OffsetBitSize = getLoadStoreOffsetSizeInBits(MI.getOpcode());
+    unsigned OffsetBitSize =
+        getLoadStoreOffsetSizeInBits(MI.getOpcode(), MI.getOperand(OpNo - 1));
     unsigned OffsetAlign = getLoadStoreOffsetAlign(MI.getOpcode());
 
     if (OffsetBitSize < 16 && isInt<16>(Offset) &&
         (!isIntN(OffsetBitSize, Offset) ||
          OffsetToAlignment(Offset, OffsetAlign) != 0)) {
       // If we have an offset that needs to fit into a signed n-bit immediate
       // (where n < 16) and doesn't, but does fit into 16-bits then use an ADDiu
       MachineBasicBlock &MBB = *MI.getParent();

test/CodeGen/Mips/inlineasm-constraint_ZC_2.ll

+; RUN: llc -march=mips -mcpu=mips32r6 < %s | FileCheck %s
+; RUN: llc -march=mips -mcpu=mips64r6 -target-abi=n64 < %s | FileCheck %s
+
+%struct.anon = type { [63 x i32], i32, i32 }
+
+define i32 @Atomic() {
+; CHECK-LABEL: Atomic:
+entry:
+  %s = alloca %struct.anon, align 4
+  %0 = bitcast %struct.anon* %s to i8*
+  %count = getelementptr inbounds %struct.anon, %struct.anon* %s, i64 0, i32 1
+  store i32 0, i32* %count, align 4
+
+; CHECK: #APP
+; CHECK: ll ${{[0-9]+}}, 0(${{[0-9a-z]+}})
+; CHECK: sc ${{[0-9]+}}, 0(${{[0-9a-z]+}})
+; CHECK: #NO_APP
+
+  %1 = call { i32, i32 } asm sideeffect ".set push\0A.set noreorder\0A1:\0All $0, $2\0Aaddu $1, $0, $3\0Asc $1, $2\0Abeqz $1, 1b\0Aaddu $1, $0, $3\0A.set pop\0A", "=&r,=&r,=*^ZC,Ir,*^ZC,~{memory},~{$1}"(i32* %count, i32 10, i32* %count)
+  %asmresult1.i = extractvalue { i32, i32 } %1, 1
+  %cmp = icmp ne i32 %asmresult1.i, 10
+  %conv = zext i1 %cmp to i32
+  %call2 = call i32 @f(i32 signext %conv)
+  ret i32 %call2
+}
+
+declare i32 @f(i32 signext)