[RemoveDIs][NFC] Add DPLabel class [2/3]

[OCHyams]

Patch 2 of 3 to add llvm.dbg.label support to the RemoveDIs project. The
patch stack adds the DPLabel class, which is the RemoveDIs llvm.dbg.label
equivalent.

   1. Add DbgRecord base class for DPValue and the not-yet-added
      DPLabel class.
-> 2. Add the DPLabel class.
   3. Enable dbg.label conversion and add support to passes.

This will be used (and tested) in the final patch, coming next.

---

Take the opportunity to refactor of isEquivalentTo to avoid some unnecessary duplication.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-debuginfo

Author: Orlando Cazalet-Hyams (OCHyams)

Changes

Patch 2 of 3 to add llvm.dbg.label support to the RemoveDIs project. The
patch stack adds the DPLabel class, which is the RemoveDIs llvm.dbg.label
equivalent.

   1. Add DbgRecord base class for DPValue and the not-yet-added
      DPLabel class.
-> 2. Add the DPLabel class.
   3. Enable dbg.label conversion and add support to passes.

This will be used (and tested) in the final patch, coming next.

Take the opportunity to refactor of isEquivalentTo to avoid some unnecessary duplication.

---

Full diff: https://github.com/llvm/llvm-project/pull/82376.diff

3 Files Affected:

• (modified) llvm/include/llvm/IR/DebugProgramInstruction.h (+29-3)
• (modified) llvm/lib/IR/AsmWriter.cpp (+37-2)
• (modified) llvm/lib/IR/DebugProgramInstruction.cpp (+17-9)

diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h
index 1fa6b6f519640e..71b3b48adab2be 100644
--- a/llvm/include/llvm/IR/DebugProgramInstruction.h
+++ b/llvm/include/llvm/IR/DebugProgramInstruction.h
@@ -79,14 +79,13 @@ class raw_ostream;
 ///   deleteRecord
 ///   clone
 ///   isIdenticalToWhenDefined
-///   isEquivalentTo
 ///   both print methods
 class DbgRecord : public ilist_node<DbgRecord> {
 public:
   /// Marker that this DbgRecord is linked into.
   DPMarker *Marker = nullptr;
   /// Subclass discriminator.
-  enum Kind : uint8_t { ValueKind };
+  enum Kind : uint8_t { ValueKind, LabelKind };
 
 protected:
   DebugLoc DbgLoc;
@@ -104,9 +103,11 @@ class DbgRecord : public ilist_node<DbgRecord> {
   void print(raw_ostream &O, bool IsForDebug = false) const;
   void print(raw_ostream &O, ModuleSlotTracker &MST, bool IsForDebug) const;
   bool isIdenticalToWhenDefined(const DbgRecord &R) const;
-  bool isEquivalentTo(const DbgRecord &R) const;
   ///@}
 
+  /// Same as isIdenticalToWhenDefined but checks DebugLoc too.
+  bool isEquivalentTo(const DbgRecord &R) const;
+
   Kind getRecordKind() const { return RecordKind; }
 
   void setMarker(DPMarker *M) { Marker = M; }
@@ -156,6 +157,31 @@ class DbgRecord : public ilist_node<DbgRecord> {
   ~DbgRecord() = default;
 };
 
+/// Records a position in IR for a source label (DILabel). Corresponds to the
+/// llvm.dbg.label intrinsic.
+/// FIXME: Rename DbgLabelRecord when DPValue is renamed to DbgVariableRecord.
+class DPLabel : public DbgRecord {
+  DILabel *Label;
+
+public:
+  DPLabel(DILabel *Label, DebugLoc DL)
+      : DbgRecord(LabelKind, DL), Label(Label) {
+    assert(Label && "unexpected nullptr");
+  }
+
+  DPLabel *clone() const;
+  void print(raw_ostream &O, bool IsForDebug = false) const;
+  void print(raw_ostream &ROS, ModuleSlotTracker &MST, bool IsForDebug) const;
+
+  void setLabel(DILabel *NewLabel) { Label = NewLabel; }
+  DILabel *getLabel() const { return Label; }
+
+  /// Support type inquiry through isa, cast, and dyn_cast.
+  static bool classof(const DbgRecord *E) {
+    return E->getRecordKind() == LabelKind;
+  }
+};
+
 /// Record of a variable value-assignment, aka a non instruction representation
 /// of the dbg.value intrinsic.
 ///
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
index 251485a403fee6..0cd7a2d2dd57a1 100644
--- a/llvm/lib/IR/AsmWriter.cpp
+++ b/llvm/lib/IR/AsmWriter.cpp
@@ -292,8 +292,8 @@ static const Module *getModuleFromDPI(const DPMarker *Marker) {
   return M ? M->getParent() : nullptr;
 }
 
-static const Module *getModuleFromDPI(const DPValue *DPV) {
-  return DPV->getMarker() ? getModuleFromDPI(DPV->getMarker()) : nullptr;
+static const Module *getModuleFromDPI(const DbgRecord *DR) {
+  return DR->getMarker() ? getModuleFromDPI(DR->getMarker()) : nullptr;
 }
 
 static void PrintCallingConv(unsigned cc, raw_ostream &Out) {
@@ -2676,6 +2676,7 @@ class AssemblyWriter {
   void printInstruction(const Instruction &I);
   void printDPMarker(const DPMarker &DPI);
   void printDPValue(const DPValue &DPI);
+  void printDPLabel(const DPLabel &DPL);
   void printDbgRecord(const DbgRecord &DPI);
 
   void printUseListOrder(const Value *V, const std::vector<unsigned> &Shuffle);
@@ -4622,6 +4623,16 @@ void AssemblyWriter::printDPValue(const DPValue &Value) {
   Out << " }";
 }
 
+void AssemblyWriter::printDPLabel(const DPLabel &Label) {
+  // There's no formal representation of a DPLabel -- print purely as
+  // a debugging aid.
+  Out << "  DPLabel { ";
+  auto WriterCtx = getContext();
+  WriteAsOperandInternal(Out, Label.getLabel(), WriterCtx, true);
+  Out << " marker @" << Label.getMarker();
+  Out << " }";
+}
+
 void AssemblyWriter::printMetadataAttachments(
     const SmallVectorImpl<std::pair<unsigned, MDNode *>> &MDs,
     StringRef Separator) {
@@ -4885,6 +4896,12 @@ void DPMarker::print(raw_ostream &ROS, ModuleSlotTracker &MST,
   W.printDPMarker(*this);
 }
 
+void DPLabel::print(raw_ostream &ROS, bool IsForDebug) const {
+
+  ModuleSlotTracker MST(getModuleFromDPI(this), true);
+  print(ROS, MST, IsForDebug);
+}
+
 void DPValue::print(raw_ostream &ROS, ModuleSlotTracker &MST,
                     bool IsForDebug) const {
   // There's no formal representation of a DPValue -- print purely as a
@@ -4904,6 +4921,24 @@ void DPValue::print(raw_ostream &ROS, ModuleSlotTracker &MST,
   W.printDPValue(*this);
 }
 
+void DPLabel::print(raw_ostream &ROS, ModuleSlotTracker &MST,
+                    bool IsForDebug) const {
+  // There's no formal representation of a DbgLabelRecord -- print purely as
+  // a debugging aid.
+  formatted_raw_ostream OS(ROS);
+  SlotTracker EmptySlotTable(static_cast<const Module *>(nullptr));
+  SlotTracker &SlotTable =
+      MST.getMachine() ? *MST.getMachine() : EmptySlotTable;
+  auto incorporateFunction = [&](const Function *F) {
+    if (F)
+      MST.incorporateFunction(*F);
+  };
+  incorporateFunction(Marker->getParent() ? Marker->getParent()->getParent()
+                                          : nullptr);
+  AssemblyWriter W(OS, SlotTable, getModuleFromDPI(this), nullptr, IsForDebug);
+  W.printDPLabel(*this);
+}
+
 void Value::print(raw_ostream &ROS, bool IsForDebug) const {
   bool ShouldInitializeAllMetadata = false;
   if (auto *I = dyn_cast<Instruction>(this))
diff --git a/llvm/lib/IR/DebugProgramInstruction.cpp b/llvm/lib/IR/DebugProgramInstruction.cpp
index 9568b2257e61a0..028538d7413e02 100644
--- a/llvm/lib/IR/DebugProgramInstruction.cpp
+++ b/llvm/lib/IR/DebugProgramInstruction.cpp
@@ -64,6 +64,9 @@ void DbgRecord::deleteRecord() {
   case ValueKind:
     delete cast<DPValue>(this);
     break;
+  case LabelKind:
+    delete cast<DPLabel>(this);
+    break;
   default:
     llvm_unreachable("unsupported DbgRecord kind");
   }
@@ -74,6 +77,9 @@ void DbgRecord::print(raw_ostream &O, bool IsForDebug) const {
   case ValueKind:
     cast<DPValue>(this)->print(O, IsForDebug);
     break;
+  case LabelKind:
+    cast<DPLabel>(this)->print(O, IsForDebug);
+    break;
   default:
     llvm_unreachable("unsupported DbgRecord kind");
   };
@@ -85,6 +91,9 @@ void DbgRecord::print(raw_ostream &O, ModuleSlotTracker &MST,
   case ValueKind:
     cast<DPValue>(this)->print(O, MST, IsForDebug);
     break;
+  case LabelKind:
+    cast<DPLabel>(this)->print(O, MST, IsForDebug);
+    break;
   default:
     llvm_unreachable("unsupported DbgRecord kind");
   };
@@ -97,21 +106,16 @@ bool DbgRecord::isIdenticalToWhenDefined(const DbgRecord &R) const {
   case ValueKind:
     return cast<DPValue>(this)->isIdenticalToWhenDefined(*cast<DPValue>(&R));
     break;
+  case LabelKind:
+    return cast<DPLabel>(this)->getLabel() == cast<DPLabel>(R).getLabel();
+    break;
   default:
     llvm_unreachable("unsupported DbgRecord kind");
   };
 }
 
 bool DbgRecord::isEquivalentTo(const DbgRecord &R) const {
-  if (RecordKind != R.RecordKind)
-    return false;
-  switch (RecordKind) {
-  case ValueKind:
-    return cast<DPValue>(this)->isEquivalentTo(*cast<DPValue>(&R));
-    break;
-  default:
-    llvm_unreachable("unsupported DbgRecord kind");
-  };
+  return getDebugLoc() == R.getDebugLoc() && isIdenticalToWhenDefined(R);
 }
 
 DPValue *DPValue::createDPValue(Value *Location, DILocalVariable *DV,
@@ -314,6 +318,8 @@ DbgRecord *DbgRecord::clone() const {
   switch (RecordKind) {
   case ValueKind:
     return cast<DPValue>(this)->clone();
+  case LabelKind:
+    return cast<DPLabel>(this)->clone();
   default:
     llvm_unreachable("unsupported DbgRecord kind");
   };
@@ -321,6 +327,8 @@ DbgRecord *DbgRecord::clone() const {
 
 DPValue *DPValue::clone() const { return new DPValue(*this); }
 
+DPLabel *DPLabel::clone() const { return new DPLabel(Label, getDebugLoc()); }
+
 DbgVariableIntrinsic *
 DPValue::createDebugIntrinsic(Module *M, Instruction *InsertBefore) const {
   [[maybe_unused]] DICompileUnit *Unit =

[SLTozer]

Largely LGTM, just a couple comments.

[SLTozer]

Suggested change

	assert(Label && "unexpected nullptr");
	assert(Label && "Unexpected nullptr");

[SLTozer]

printDbgRecord should be updated to call through to printDPLabel, right?

[SLTozer]

At the risk of being a pain, should we make this an else if (<Label Cast>) followed by an llvm_unreachable? As a guard against future types being added and missed here.

[OCHyams]

The cast would catch that, but I can do so if you'd prefer. The unreachable has a nicer error message than cast failure (and is what I've done elsewhere). Wdyt?

[SLTozer]

I'd prefer the unreachable, since it's clearer and more consistent with the approach elsewhere - imo at least.

[OCHyams]

I said I'd done the unreachable approach elsewhere, but I've actually done a mix (in future patches) of whatever looks right at the site. I'll go with unreachable here as there's not much in it, but I might push back against this pattern in the next patches - in some cases it's much cleaner to just cast (imo), but we can discuss that in those patches!