[RFC] [DRAFT] [IR] Add instruction IDs to simplify debugging.

[ygribov]

Summary

This provides an easy way to map from instruction in dumpfile to a place where it's created or modified in the compiler (by using conditional breakpoints in debugger).

[ygribov]

Discussion in Discourse: https://discourse.llvm.org/t/adding-instruction-ids-in-debug-dumps/68188

[nhaehnle]

These data structures have been designed quite carefully with size in mind, so I am against making these debug-only additions. The change as-is is also not thread-safe.

Have you tried just attaching metadata to instructions instead?

[ygribov]

In D143321#4106526, @nhaehnle wrote:

These data structures have been designed quite carefully with size in mind, so I am against making these debug-only additions.

Right. I wonder if separate Cmake switch could alleviate this.

The change as-is is also not thread-safe.

That's true. What's worse, I'm afraid it will be inherently racy in parallel compilations.

Have you tried just attaching metadata to instructions instead?

I haven't, thanks for the suggestion.

llvm/include/llvm/CodeGen/MachineInstr.h

PointerSumType<ExtraInfoInlineKinds,
       Info;
 
   DebugLoc DbgLoc; // Source line information.
 
   /// Unique instruction number. Used by DBG_INSTR_REFs to refer to the values
   /// defined by this instruction.
   unsigned DebugInstrNum;
 
+#ifndef NDEBUG
+  unsigned UID;
+  static unsigned NextUID;
+#endif
+
+  void setUID();
+
   // Intrusive list support
   friend struct ilist_traits<MachineInstr>;
   friend struct ilist_callback_traits<MachineBasicBlock>;
   void setParent(MachineBasicBlock *P) { Parent = P; }
 
   /// This constructor creates a copy of the given
   /// MachineInstr in the given MachineFunction.
   MachineInstr(MachineFunction &, const MachineInstr &);

llvm/include/llvm/IR/Value.h

protected:
   // Use the same type as the bitfield above so that MSVC will pack them.
   unsigned IsUsedByMD : 1;
   unsigned HasName : 1;
   unsigned HasMetadata : 1; // Has metadata attached to this?
   unsigned HasHungOffUses : 1;
   unsigned HasDescriptor : 1;
 
 private:
+
+#ifndef NDEBUG
+  unsigned UID;
+  static unsigned NextUID;
+#endif
+
+  void setUID();
+
   template <typename UseT> // UseT == 'Use' or 'const Use'
   class use_iterator_impl {
     friend class Value;
 
     UseT *U;
 
     explicit use_iterator_impl(UseT *u) : U(u) {}
 
protected:
   /// destructor has been protected, and the caller should manually call
   /// deleteValue.
   ~Value(); // Use deleteValue() to delete a generic Value.
 
 public:
   Value(const Value &) = delete;
   Value &operator=(const Value &) = delete;
 
+#ifndef NDEBUG
+  /// Returns unique ID of Value
+  unsigned getUID() const { return UID; }
+#endif
+
   /// Delete a pointer to a generic Value.
   void deleteValue();
 
   /// Support for debugging, callable in GDB: V->dump()
   void dump() const;
 
   /// Implement operator<< on Value.
   /// @{

llvm/lib/CodeGen/MachineInstr.cpp

 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <cstring>
 #include <utility>
 
 using namespace llvm;
 
+extern cl::opt<bool> PrintUIDs;
+
 static const MachineFunction *getMFIfAvailable(const MachineInstr &MI) {
   if (const MachineBasicBlock *MBB = MI.getParent())
     if (const MachineFunction *MF = MBB->getParent())
       return MF;
   return nullptr;
 }
 
 // Try to crawl up to the machine function and get TRI and IntrinsicInfo from
MachineInstr::MachineInstr(MachineFunction &MF, const MCInstrDesc &TID,
   if (unsigned NumOps = MCID->getNumOperands() + MCID->implicit_defs().size() +
                         MCID->implicit_uses().size()) {
     CapOperands = OperandCapacity::get(NumOps);
     Operands = MF.allocateOperandArray(CapOperands);
   }
 
   if (!NoImp)
     addImplicitDefUseOperands(MF);
+
+  setUID();
 }
 
 /// MachineInstr ctor - Copies MachineInstr arg exactly.
 /// Does not copy the number from debug instruction numbering, to preserve
 /// uniqueness.
 MachineInstr::MachineInstr(MachineFunction &MF, const MachineInstr &MI)
     : MCID(&MI.getDesc()), Info(MI.Info), DbgLoc(MI.getDebugLoc()),
       DebugInstrNum(0) {
MachineInstr::MachineInstr(MachineFunction &MF, const MachineInstr &MI)
   for (unsigned i = 0, e = getNumOperands(); i < e; ++i) {
     MachineOperand &NewMO = getOperand(i);
     const MachineOperand &OrigMO = MI.getOperand(i);
     NewMO.TiedTo = OrigMO.TiedTo;
   }
 
   // Copy all the sensible flags.
   setFlags(MI.Flags);
+
+  setUID();
+}
+
+void MachineInstr::setUID() {
+#ifndef NDEBUG
+  UID = NextUID;
+  ++NextUID;
+#endif
 }
 
+unsigned MachineInstr::NextUID;
+
 void MachineInstr::moveBefore(MachineInstr *MovePos) {
   MovePos->getParent()->splice(MovePos, getParent(), getIterator());
 }
 
 /// getRegInfo - If this instruction is embedded into a MachineFunction,
 /// return the MachineRegisterInfo object for the current function, otherwise
 /// return null.
 MachineRegisterInfo *MachineInstr::getRegInfo() {
if (isDebugValue() && getDebugVariableOp().isMetadata()) {
     }
     auto *DV = getDebugVariable();
     OS << " line no:" <<  DV->getLine();
     if (isIndirectDebugValue())
       OS << " indirect";
   }
   // TODO: DBG_LABEL
 
+  if (PrintUIDs) {
+#ifndef NDEBUG
+    if (!HaveSemi) {
+      OS << ";";
+      HaveSemi = true;
+    }
+    OS << " UID:" << UID;
+#endif
+  }
+
   if (AddNewLine)
     OS << '\n';
 }
 
 bool MachineInstr::addRegisterKilled(Register IncomingReg,
                                      const TargetRegisterInfo *RegInfo,
                                      bool AddIfNotFound) {
   bool isPhysReg = IncomingReg.isPhysical();

llvm/lib/IR/AsmWriter.cpp

 #include <optional>
 #include <string>
 #include <tuple>
 #include <utility>
 #include <vector>
 
 using namespace llvm;
 
+cl::opt<bool> PrintUIDs("print-uids", cl::Hidden, cl::init(false),
+                        cl::desc("Print instruction IDs in IR dumps"));
+
 // Make virtual table appear in this compilation unit.
 AssemblyAnnotationWriter::~AssemblyAnnotationWriter() = default;
 
 //===----------------------------------------------------------------------===//
 // Helper Functions
 //===----------------------------------------------------------------------===//
 
 using OrderMap = MapVector<const Value *, unsigned>;

 /// printInfoComment - Print a little comment after the instruction indicating
 /// which slot it occupies.
 void AssemblyWriter::printInfoComment(const Value &V) {
   if (const auto *Relocate = dyn_cast<GCRelocateInst>(&V))
     printGCRelocateComment(*Relocate);
 
   if (AnnotationWriter)
     AnnotationWriter->printInfoComment(V, Out);
+
+  if (PrintUIDs) {
+#ifndef NDEBUG
+    Out << " ; UID:";
+    Out << V.getUID();
+#endif
+  }
 }
 
 static void maybePrintCallAddrSpace(const Value *Operand, const Instruction *I,
                                     raw_ostream &Out) {
   // We print the address space of the call if it is non-zero.
   if (Operand == nullptr) {
     Out << " <cannot get addrspace!>";
     return;

llvm/lib/IR/Value.cpp

Value::Value(Type *ty, unsigned scid)
   if (OpCode == Instruction::Call || OpCode == Instruction::Invoke ||
       OpCode == Instruction::CallBr)
     assert((VTy->isFirstClassType() || VTy->isVoidTy() || VTy->isStructTy()) &&
            "invalid CallBase type!");
   else if (SubclassID != BasicBlockVal &&
            (/*SubclassID < ConstantFirstVal ||*/ SubclassID > ConstantLastVal))
     assert((VTy->isFirstClassType() || VTy->isVoidTy()) &&
            "Cannot create non-first-class values except for constants!");
-  static_assert(sizeof(Value) == 2 * sizeof(void *) + 2 * sizeof(unsigned),
+#ifdef NDEBUG
+  constexpr bool HasUID = false;
+#else
+  constexpr bool HasUID = true;
+#endif
+  static_assert(sizeof(Value) == 2 * sizeof(void *) + 2 * sizeof(unsigned) +
+                                     (HasUID ? sizeof(void *) : 0),
                 "Value too big");
+  setUID();
 }
 
 Value::~Value() {
   // Notify all ValueHandles (if present) that this value is going away.
   if (HasValueHandle)
     ValueHandleBase::ValueIsDeleted(this);
   if (isUsedByMetadata())
     ValueAsMetadata::handleDeletion(this);

 #endif
   assert(materialized_use_empty() && "Uses remain when a value is destroyed!");
 
   // If this value is named, destroy the name.  This should not be in a symtab
   // at this point.
   destroyValueName();
 }
 
+void Value::setUID() {
+#ifndef NDEBUG
+  UID = NextUID;
+  ++NextUID;
+#endif
+}
+
+unsigned Value::NextUID;
+
 void Value::deleteValue() {
   switch (getValueID()) {
 #define HANDLE_VALUE(Name)                                                     \
   case Value::Name##Val:                                                       \
     delete static_cast<Name *>(this);                                          \
     break;
 #define HANDLE_MEMORY_VALUE(Name)                                              \
   case Value::Name##Val:                                                       \