Print target inline asm memory constraints

[borisboesler]

Summary

In LLVM target dependent inline asm constraints can be defined - for registers and memories. But if MachineInstrs are printed, LLVM fails in InlineAsm::getMemConstraintName() to print target inline asm memory constraints.
With this patch memory constraints can be defined starting at InlineAsm::Constraints_Max + 1 and can be printed by overwriting virtual method TargetInstrInfo::getInlineAsmMemConstraintName.

[borisboesler]

Change to follow lint rules

[kschwarz]

llvm/include/llvm/IR/InlineAsm.h

266

As far as I understand, upstream targets define their (target) specific memory constraints in this common enum.
The Constraints_Max value is used in the assertion to make sure all upstream constraints are handled properly.

Why do you need to remove the assertion upstream? I would rather expect your downstream target to add more values to this enum, and to handle them in getMemConstraintName.

[borisboesler]

llvm/include/llvm/IR/InlineAsm.h

266

Why should we add target related things in llvm/IR? We do not add target opcodes in llvm/IR/ISDOpcodes.h. I propose to keep target related code in their target codebase.

[dsanders]

llvm/include/llvm/IR/InlineAsm.h

266

As far as I understand, upstream targets define their (target) specific memory constraints in this common enum.

That's true at the moment but as far as I can remember there was no particularly good reason for it to be here rather than in the target (for example, AFAICT the numbers aren't used in the bitcode format). There just wasn't anything justifying the extra plumbing.

For in-tree targets there isn't really a practical reason to move it to the target but I agree it belongs there in principle. For out-of-tree targets there's some small but nice befits for both merge pain and binary compatibility (e.g. a shared library adding a new target to an existing LLVM)

LGTM

[borisboesler]

llvm/include/llvm/IR/InlineAsm.h

266

As far as I understand, upstream targets define their (target) specific memory constraints in this common enum.

That's true at the moment but as far as I can remember there was no particularly good reason for it to be here rather than in the target (for example, AFAICT the numbers aren't used in the bitcode format). There just wasn't anything justifying the extra plumbing.

For in-tree targets there isn't really a practical reason to move it to the target but I agree it belongs there in principle. For out-of-tree targets there's some small but nice befits for both merge pain and binary compatibility (e.g. a shared library adding a new target to an existing LLVM)

LGTM

Thanks. I don't have commit access. Could you commit it for me, please?

llvm/include/llvm/CodeGen/TargetInstrInfo.h

virtual bool isSchedulingBoundary(const MachineInstr &MI,
                                     const MachineFunction &MF) const;
 
   /// Measure the specified inline asm to determine an approximation of its
   /// length.
   virtual unsigned getInlineAsmLength(
     const char *Str, const MCAsmInfo &MAI,
     const TargetSubtargetInfo *STI = nullptr) const;
 
+  /// Return the target inline asm constraint name
+  virtual StringRef getInlineAsmMemConstraintName(unsigned Constraint) const;
+
   /// Allocate and return a hazard recognizer to use for this target when
   /// scheduling the machine instructions before register allocation.
   virtual ScheduleHazardRecognizer *
   CreateTargetHazardRecognizer(const TargetSubtargetInfo *STI,
                                const ScheduleDAG *DAG) const;
 
   /// Allocate and return a hazard recognizer to use for this target when
   /// scheduling the machine instructions before register allocation.

llvm/include/llvm/IR/InlineAsm.h

enum : uint32_t {
     Constraint_Us,
     Constraint_Ut,
     Constraint_Uv,
     Constraint_Uy,
     Constraint_X,
     Constraint_Z,
     Constraint_ZC,
     Constraint_Zy,
     Constraints_Max = Constraint_Zy,
     Constraints_ShiftAmount = 16,
 
     Flag_MatchingOperand = 0x80000000
   };
 
   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");
static unsigned getFlagWordForRegClass(unsigned InputFlag, unsigned RC) {
     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);
   }

llvm/lib/CodeGen/MachineInstr.cpp

if (isDebugValue() && MO.isMetadata()) {
         if (TRI) {
           OS << ':' << TRI->getRegClassName(TRI->getRegClass(RCID));
         } else
           OS << ":RC" << RCID;
       }
 
       if (InlineAsm::isMemKind(Flag)) {
         unsigned MCID = InlineAsm::getMemoryConstraintID(Flag);
-        OS << ":" << InlineAsm::getMemConstraintName(MCID);
+        if (TII)
+          OS << ":" << TII -> getInlineAsmMemConstraintName(MCID);
+        else
+          OS << ":CONSTRAINT" << MCID;
       }
 
       unsigned TiedTo = 0;
       if (InlineAsm::isUseOperandTiedToDef(Flag, TiedTo))
         OS << " tiedto:$" << TiedTo;
 
       OS << ']';
 

llvm/lib/CodeGen/TargetInstrInfo.cpp

if (AtInsnStart && !isSpace(static_cast<unsigned char>(*Str))) {
       Length += AddLength;
       AtInsnStart = false;
     }
   }
 
   return Length;
 }
 
+/// Return the target constraint name
+StringRef
+TargetInstrInfo::getInlineAsmMemConstraintName(unsigned Constraint) const {
+  return InlineAsm::getMemConstraintName(Constraint);
+}
+
 /// ReplaceTailWithBranchTo - Delete the instruction OldInst and everything
 /// after it, replacing it with an unconditional branch to NewDest.
 void
 TargetInstrInfo::ReplaceTailWithBranchTo(MachineBasicBlock::iterator Tail,
                                          MachineBasicBlock *NewDest) const {
   MachineBasicBlock *MBB = Tail->getParent();
 
   // Remove all the old successors of MBB from the CFG.
if (!InlineAsm::isImmKind(Flag) && !InlineAsm::isMemKind(Flag) &&
     if (TRI) {
       OS << ':' << TRI->getRegClassName(TRI->getRegClass(RCID));
     } else
       OS << ":RC" << RCID;
   }
 
   if (InlineAsm::isMemKind(Flag)) {
     unsigned MCID = InlineAsm::getMemoryConstraintID(Flag);
-    OS << ":" << InlineAsm::getMemConstraintName(MCID);
+    OS << ":" << getInlineAsmMemConstraintName(MCID);
   }
 
   unsigned TiedTo = 0;
   if (InlineAsm::isUseOperandTiedToDef(Flag, TiedTo))
     OS << " tiedto:$" << TiedTo;
 
   return OS.str();
 }
 
 TargetInstrInfo::PipelinerLoopInfo::~PipelinerLoopInfo() {}