Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RemoveDIs][DebugInfo] Add DPVAssign variant of DPValue #77912

Merged
merged 3 commits into from
Jan 16, 2024

Conversation

SLTozer
Copy link
Contributor

@SLTozer SLTozer commented Jan 12, 2024

This implements the DbgAssignIntrinsic class as a variant of DPValues - unfortunately this involves increasing the size of the DebugValueUser storage by 3x, but this is necessary to enable assigns to be represented, and can be offset in a future patch by splitting DPValue into subclasses such that each variant can store only the fields it needs. This patch does not actually create DPVAssigns in any case; future patches will handle this variant in all cases where generic DPValue handling does not. This patch also does not implement tracking support for DIAssignIDs, which is necessary to find DPVAssigns that reference a given DIAssignID; that is added in a subsequent patch.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 12, 2024

@llvm/pr-subscribers-llvm-ir

Author: Stephen Tozer (SLTozer)

Changes

This implements the DbgAssignIntrinsic class as a variant of DPValues - unfortunately this involves increasing the size of the DebugValueUser storage by 3x, but this is necessary to enable assigns to be represented, and can be offset in a future patch by splitting DPValue into subclasses such that each variant can store only the fields it needs. This patch does not actually create DPVAssigns in any case; future patches will handle this variant in all cases where generic DPValue handling does not. This patch also does not implement tracking support for DIAssignIDs, which is necessary to find DPVAssigns that reference a given DIAssignID; that is added in a subsequent patch.


Patch is 21.60 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/77912.diff

5 Files Affected:

  • (modified) llvm/include/llvm/IR/DebugProgramInstruction.h (+52-7)
  • (modified) llvm/include/llvm/IR/Metadata.h (+42-28)
  • (modified) llvm/lib/IR/AsmWriter.cpp (+27-1)
  • (modified) llvm/lib/IR/DebugProgramInstruction.cpp (+105-16)
  • (modified) llvm/lib/IR/Metadata.cpp (+36-15)
diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h
index 8230070343e0c1..66453c1a2c1a5e 100644
--- a/llvm/include/llvm/IR/DebugProgramInstruction.h
+++ b/llvm/include/llvm/IR/DebugProgramInstruction.h
@@ -59,6 +59,7 @@ class BasicBlock;
 class MDNode;
 class Module;
 class DbgVariableIntrinsic;
+class DIAssignID;
 class DPMarker;
 class DPValue;
 class raw_ostream;
@@ -81,6 +82,7 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
   DILocalVariable *Variable;
   DIExpression *Expression;
   DebugLoc DbgLoc;
+  DIExpression *AddressExpression;
 
 public:
   void deleteInstr();
@@ -97,6 +99,7 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
   enum class LocationType {
     Declare,
     Value,
+    Assign,
 
     End, ///< Marks the end of the concrete types.
     Any, ///< To indicate all LocationTypes in searches.
@@ -117,6 +120,22 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
   /// assigning \p Location to the DV / Expr / DI variable.
   DPValue(Metadata *Location, DILocalVariable *DV, DIExpression *Expr,
           const DILocation *DI, LocationType Type = LocationType::Value);
+  DPValue(Metadata *Value, DILocalVariable *Variable, DIExpression *Expression,
+          DIAssignID *AssignID, Metadata *Address,
+          DIExpression *AddressExpression, const DILocation *DI);
+
+  static DPValue *createDPVAssign(Metadata *Value, DILocalVariable *Variable,
+                                  DIExpression *Expression,
+                                  DIAssignID *AssignID, Metadata *Address,
+                                  DIExpression *AddressExpression,
+                                  const DILocation *DI,
+                                  Instruction *InsertBefore = nullptr);
+  static DPValue *createLinkedDPVAssign(Instruction *LinkedInstr, Value *Val,
+                                        DILocalVariable *Variable,
+                                        DIExpression *Expression,
+                                        Value *Address,
+                                        DIExpression *AddressExpression,
+                                        const DILocation *DI);
 
   /// Iterator for ValueAsMetadata that internally uses direct pointer iteration
   /// over either a ValueAsMetadata* or a ValueAsMetadata**, dereferencing to the
@@ -194,7 +213,7 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
 
   /// Does this describe the address of a local variable. True for dbg.addr
   /// and dbg.declare, but not dbg.value, which describes its value.
-  bool isAddressOfVariable() const { return Type != LocationType::Value; }
+  bool isAddressOfVariable() const { return Type == LocationType::Declare; }
   LocationType getType() const { return Type; }
 
   DebugLoc getDebugLoc() const { return DbgLoc; }
@@ -207,7 +226,11 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
 
   DIExpression *getExpression() const { return Expression; }
 
-  Metadata *getRawLocation() const { return DebugValue; }
+  /// Returns the metadata operand for the first location description. i.e.,
+  /// dbg intrinsic dbg.value,declare operand and dbg.assign 1st location
+  /// operand (the "value componenet"). Note the operand (singular) may be
+  /// a DIArgList which is a list of values.
+  Metadata *getRawLocation() const { return DebugValues[0]; }
 
   /// Use of this should generally be avoided; instead,
   /// replaceVariableLocationOp and addVariableLocationOps should be used where
@@ -217,23 +240,45 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
         (isa<ValueAsMetadata>(NewLocation) || isa<DIArgList>(NewLocation) ||
          isa<MDNode>(NewLocation)) &&
         "Location for a DPValue must be either ValueAsMetadata or DIArgList");
-    resetDebugValue(NewLocation);
+    resetDebugValue(0, NewLocation);
   }
 
   /// Get the size (in bits) of the variable, or fragment of the variable that
   /// is described.
   std::optional<uint64_t> getFragmentSizeInBits() const;
 
+  /// @name DbgAssign Methods
+  /// @{
+  bool isDbgAssign() const { return getType() == LocationType::Assign; }
+
+  Value *getAddress() const;
+  Metadata *getRawAddress() const {
+    return isDbgAssign() ? DebugValues[1] : DebugValues[0];
+  }
+  Metadata *getRawAssignID() const { return DebugValues[2]; }
+  DIAssignID *getAssignID() const;
+  DIExpression *getAddressExpression() const { return AddressExpression; }
+  void setAddressExpression(DIExpression *NewExpr) {
+    AddressExpression = NewExpr;
+  }
+  void setAssignId(DIAssignID *New);
+  void setAddress(Value *V) { resetDebugValue(1, ValueAsMetadata::get(V)); }
+  /// Kill the address component.
+  void setKillAddress();
+  /// Check whether this kills the address component. This doesn't take into
+  /// account the position of the intrinsic, therefore a returned value of false
+  /// does not guarentee the address is a valid location for the variable at the
+  /// intrinsic's position in IR.
+  bool isKillAddress() const;
+
+  /// @}
+public:
   DPValue *clone() const;
   /// Convert this DPValue back into a dbg.value intrinsic.
   /// \p InsertBefore Optional position to insert this intrinsic.
   /// \returns A new dbg.value intrinsic representiung this DPValue.
   DbgVariableIntrinsic *createDebugIntrinsic(Module *M,
                                              Instruction *InsertBefore) const;
-  /// Handle changes to the location of the Value(s) that we refer to happening
-  /// "under our feet".
-  void handleChangedLocation(Metadata *NewLocation);
-
   void setMarker(DPMarker *M) { Marker = M; }
 
   DPMarker *getMarker() { return Marker; }
diff --git a/llvm/include/llvm/IR/Metadata.h b/llvm/include/llvm/IR/Metadata.h
index 4498423c4c460d..365820b95b298d 100644
--- a/llvm/include/llvm/IR/Metadata.h
+++ b/llvm/include/llvm/IR/Metadata.h
@@ -211,31 +211,39 @@ class MetadataAsValue : public Value {
 /// lookup and callback handling.
 class DebugValueUser {
 protected:
-  Metadata *DebugValue;
+  // Capacity to store 3 debug values.
+  // TODO: Not all DebugValueUser instances need all 3 elements, if we
+  // restructure the DPValue class then we can template parameterize this array
+  // size.
+  std::array<Metadata *, 3> DebugValues;
+
+  ArrayRef<Metadata *> getDebugValues() const { return DebugValues; }
 
 public:
   DPValue *getUser();
   const DPValue *getUser() const;
-  void handleChangedValue(Metadata *NewDebugValue);
+  void handleChangedValue(void *Old, Metadata *NewDebugValue);
   DebugValueUser() = default;
-  explicit DebugValueUser(Metadata *DebugValue) : DebugValue(DebugValue) {
-    trackDebugValue();
+  explicit DebugValueUser(std::array<Metadata *, 3> DebugValues)
+      : DebugValues(DebugValues) {
+    trackDebugValues();
   }
-
-  DebugValueUser(DebugValueUser &&X) : DebugValue(X.DebugValue) {
-    retrackDebugValue(X);
+  DebugValueUser(DebugValueUser &&X) {
+    DebugValues = X.DebugValues;
+    retrackDebugValues(X);
   }
-  DebugValueUser(const DebugValueUser &X) : DebugValue(X.DebugValue) {
-    trackDebugValue();
+  DebugValueUser(const DebugValueUser &X) {
+    DebugValues = X.DebugValues;
+    trackDebugValues();
   }
 
   DebugValueUser &operator=(DebugValueUser &&X) {
     if (&X == this)
       return *this;
 
-    untrackDebugValue();
-    DebugValue = X.DebugValue;
-    retrackDebugValue(X);
+    untrackDebugValues();
+    DebugValues = X.DebugValues;
+    retrackDebugValues(X);
     return *this;
   }
 
@@ -243,35 +251,41 @@ class DebugValueUser {
     if (&X == this)
       return *this;
 
-    untrackDebugValue();
-    DebugValue = X.DebugValue;
-    trackDebugValue();
+    untrackDebugValues();
+    DebugValues = X.DebugValues;
+    trackDebugValues();
     return *this;
   }
 
-  ~DebugValueUser() { untrackDebugValue(); }
+  ~DebugValueUser() { untrackDebugValues(); }
 
-  void resetDebugValue() {
-    untrackDebugValue();
-    DebugValue = nullptr;
+  void resetDebugValues() {
+    untrackDebugValues();
+    DebugValues.fill(nullptr);
   }
-  void resetDebugValue(Metadata *DebugValue) {
-    untrackDebugValue();
-    this->DebugValue = DebugValue;
-    trackDebugValue();
+
+  void resetDebugValue(size_t Idx, Metadata *DebugValue) {
+    assert(Idx < 3 && "Invalid debug value index.");
+    untrackDebugValue(Idx);
+    DebugValues[Idx] = DebugValue;
+    trackDebugValue(Idx);
   }
 
   bool operator==(const DebugValueUser &X) const {
-    return DebugValue == X.DebugValue;
+    return DebugValues == X.DebugValues;
   }
   bool operator!=(const DebugValueUser &X) const {
-    return DebugValue != X.DebugValue;
+    return DebugValues != X.DebugValues;
   }
 
 private:
-  void trackDebugValue();
-  void untrackDebugValue();
-  void retrackDebugValue(DebugValueUser &X);
+  void trackDebugValue(size_t Idx);
+  void trackDebugValues();
+
+  void untrackDebugValue(size_t Idx);
+  void untrackDebugValues();
+
+  void retrackDebugValues(DebugValueUser &X);
 };
 
 /// API for tracking metadata references through RAUW and deletion.
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
index 278cdfce411050..3c15784a0ed5eb 100644
--- a/llvm/lib/IR/AsmWriter.cpp
+++ b/llvm/lib/IR/AsmWriter.cpp
@@ -1140,6 +1140,9 @@ void SlotTracker::processFunctionMetadata(const Function &F) {
 void SlotTracker::processDPValueMetadata(const DPValue &DPV) {
   CreateMetadataSlot(DPV.getVariable());
   CreateMetadataSlot(DPV.getDebugLoc());
+  if (DPV.isDbgAssign()) {
+    CreateMetadataSlot(DPV.getAssignID());
+  }
 }
 
 void SlotTracker::processInstructionMetadata(const Instruction &I) {
@@ -4571,7 +4574,22 @@ void AssemblyWriter::printDPMarker(const DPMarker &Marker) {
 void AssemblyWriter::printDPValue(const DPValue &Value) {
   // There's no formal representation of a DPValue -- print purely as a
   // debugging aid.
-  Out << "  DPValue { ";
+  Out << "  DPValue ";
+
+  switch (Value.getType()) {
+  case DPValue::LocationType::Value:
+    Out << "value";
+    break;
+  case DPValue::LocationType::Declare:
+    Out << "declare";
+    break;
+  case DPValue::LocationType::Assign:
+    Out << "assign";
+    break;
+  default:
+    llvm_unreachable("Tried to print a DPValue with an invalid LocationType!");
+  }
+  Out << " { ";
   auto WriterCtx = getContext();
   WriteAsOperandInternal(Out, Value.getRawLocation(), WriterCtx, true);
   Out << ", ";
@@ -4579,6 +4597,14 @@ void AssemblyWriter::printDPValue(const DPValue &Value) {
   Out << ", ";
   WriteAsOperandInternal(Out, Value.getExpression(), WriterCtx, true);
   Out << ", ";
+  if (Value.isDbgAssign()) {
+    WriteAsOperandInternal(Out, Value.getAssignID(), WriterCtx, true);
+    Out << ", ";
+    WriteAsOperandInternal(Out, Value.getRawAddress(), WriterCtx, true);
+    Out << ", ";
+    WriteAsOperandInternal(Out, Value.getAddressExpression(), WriterCtx, true);
+    Out << ", ";
+  }
   WriteAsOperandInternal(Out, Value.getDebugLoc().get(), WriterCtx, true);
   Out << " marker @" << Value.getMarker();
   Out << " }";
diff --git a/llvm/lib/IR/DebugProgramInstruction.cpp b/llvm/lib/IR/DebugProgramInstruction.cpp
index 7b709a2de0335f..1c1c965b1c09d7 100644
--- a/llvm/lib/IR/DebugProgramInstruction.cpp
+++ b/llvm/lib/IR/DebugProgramInstruction.cpp
@@ -14,8 +14,9 @@
 namespace llvm {
 
 DPValue::DPValue(const DbgVariableIntrinsic *DVI)
-    : DebugValueUser(DVI->getRawLocation()), Variable(DVI->getVariable()),
-      Expression(DVI->getExpression()), DbgLoc(DVI->getDebugLoc()) {
+    : DebugValueUser({DVI->getRawLocation(), nullptr, nullptr}),
+      Variable(DVI->getVariable()), Expression(DVI->getExpression()),
+      DbgLoc(DVI->getDebugLoc()), AddressExpression(nullptr) {
   switch (DVI->getIntrinsicID()) {
   case Intrinsic::dbg_value:
     Type = LocationType::Value;
@@ -23,6 +24,15 @@ DPValue::DPValue(const DbgVariableIntrinsic *DVI)
   case Intrinsic::dbg_declare:
     Type = LocationType::Declare;
     break;
+  case Intrinsic::dbg_assign: {
+    Type = LocationType::Assign;
+    const DbgAssignIntrinsic *Assign =
+        static_cast<const DbgAssignIntrinsic *>(DVI);
+    resetDebugValue(1, Assign->getRawAddress());
+    AddressExpression = Assign->getAddressExpression();
+    setAssignId(Assign->getAssignID());
+    break;
+  }
   default:
     llvm_unreachable(
         "Trying to create a DPValue with an invalid intrinsic type!");
@@ -30,17 +40,54 @@ DPValue::DPValue(const DbgVariableIntrinsic *DVI)
 }
 
 DPValue::DPValue(const DPValue &DPV)
-    : DebugValueUser(DPV.getRawLocation()),
-      Variable(DPV.getVariable()), Expression(DPV.getExpression()),
-      DbgLoc(DPV.getDebugLoc()), Type(DPV.getType()) {}
+    : DebugValueUser(DPV.DebugValues), Variable(DPV.getVariable()),
+      Expression(DPV.getExpression()), DbgLoc(DPV.getDebugLoc()),
+      AddressExpression(DPV.AddressExpression), Type(DPV.getType()) {}
 
 DPValue::DPValue(Metadata *Location, DILocalVariable *DV, DIExpression *Expr,
                  const DILocation *DI, LocationType Type)
-    : DebugValueUser(Location), Variable(DV), Expression(Expr), DbgLoc(DI),
-      Type(Type) {}
+    : DebugValueUser({Location, nullptr, nullptr}), Variable(DV),
+      Expression(Expr), DbgLoc(DI), Type(Type) {}
+
+DPValue::DPValue(Metadata *Value, DILocalVariable *Variable,
+                 DIExpression *Expression, DIAssignID *AssignID,
+                 Metadata *Address, DIExpression *AddressExpression,
+                 const DILocation *DI)
+    : DebugValueUser({Value, Address, AssignID}), Variable(Variable),
+      Expression(Expression), DbgLoc(DI), AddressExpression(AddressExpression),
+      Type(LocationType::Assign) {}
 
 void DPValue::deleteInstr() { delete this; }
 
+DPValue *DPValue::createDPVAssign(Metadata *Value, DILocalVariable *Variable,
+                                  DIExpression *Expression,
+                                  DIAssignID *AssignID, Metadata *Address,
+                                  DIExpression *AddressExpression,
+                                  const DILocation *DI,
+                                  Instruction *InsertBefore) {
+  auto *NewDPVAssign = new DPValue(Value, Variable, Expression, AssignID,
+                                   Address, AddressExpression, DI);
+  if (InsertBefore) {
+    InsertBefore->getParent()->insertDPValueBefore(NewDPVAssign,
+                                                   InsertBefore->getIterator());
+  }
+  return NewDPVAssign;
+}
+DPValue *DPValue::createLinkedDPVAssign(Instruction *LinkedInstr, Value *Val,
+                                        DILocalVariable *Variable,
+                                        DIExpression *Expression,
+                                        Value *Address,
+                                        DIExpression *AddressExpression,
+                                        const DILocation *DI) {
+  auto *Link = LinkedInstr->getMetadata(LLVMContext::MD_DIAssignID);
+  assert(Link && "Linked instruction must have DIAssign metadata attached");
+  auto *NewDPVAssign = new DPValue(
+      ValueAsMetadata::get(Val), Variable, Expression, cast<DIAssignID>(Link),
+      ValueAsMetadata::get(Address), AddressExpression, DI);
+  LinkedInstr->getParent()->insertDPValueAfter(NewDPVAssign, LinkedInstr);
+  return NewDPVAssign;
+}
+
 iterator_range<DPValue::location_op_iterator> DPValue::location_ops() const {
   auto *MD = getRawLocation();
   // If a Value has been deleted, the "location" for this DPValue will be
@@ -96,10 +143,15 @@ static ValueAsMetadata *getAsMetadata(Value *V) {
 void DPValue::replaceVariableLocationOp(Value *OldValue, Value *NewValue,
                                         bool AllowEmpty) {
   assert(NewValue && "Values must be non-null");
+
+  bool DbgAssignAddrReplaced = isDbgAssign() && OldValue == getAddress();
+  if (DbgAssignAddrReplaced)
+    setAddress(NewValue);
+
   auto Locations = location_ops();
   auto OldIt = find(Locations, OldValue);
   if (OldIt == Locations.end()) {
-    if (AllowEmpty)
+    if (AllowEmpty || DbgAssignAddrReplaced)
       return;
     llvm_unreachable("OldValue must be a current location");
   }
@@ -190,9 +242,6 @@ DPValue::createDebugIntrinsic(Module *M, Instruction *InsertBefore) const {
          "Cannot clone from BasicBlock that is not part of a Module or "
          "DICompileUnit!");
   LLVMContext &Context = getDebugLoc()->getContext();
-  Value *Args[] = {MetadataAsValue::get(Context, getRawLocation()),
-                   MetadataAsValue::get(Context, getVariable()),
-                   MetadataAsValue::get(Context, getExpression())};
   Function *IntrinsicFn;
 
   // Work out what sort of intrinsic we're going to produce.
@@ -203,16 +252,34 @@ DPValue::createDebugIntrinsic(Module *M, Instruction *InsertBefore) const {
   case DPValue::LocationType::Value:
     IntrinsicFn = Intrinsic::getDeclaration(M, Intrinsic::dbg_value);
     break;
+  case DPValue::LocationType::Assign:
+    IntrinsicFn = Intrinsic::getDeclaration(M, Intrinsic::dbg_assign);
+    break;
   case DPValue::LocationType::End:
   case DPValue::LocationType::Any:
     llvm_unreachable("Invalid LocationType");
-    break;
   }
 
   // Create the intrinsic from this DPValue's information, optionally insert
   // into the target location.
-  DbgVariableIntrinsic *DVI = cast<DbgVariableIntrinsic>(
-      CallInst::Create(IntrinsicFn->getFunctionType(), IntrinsicFn, Args));
+  DbgVariableIntrinsic *DVI;
+  if (isDbgAssign()) {
+    Value *AssignArgs[] = {
+        MetadataAsValue::get(Context, getRawLocation()),
+        MetadataAsValue::get(Context, getVariable()),
+        MetadataAsValue::get(Context, getExpression()),
+        MetadataAsValue::get(Context, getAssignID()),
+        MetadataAsValue::get(Context, getRawAddress()),
+        MetadataAsValue::get(Context, getAddressExpression())};
+    DVI = cast<DbgVariableIntrinsic>(CallInst::Create(
+        IntrinsicFn->getFunctionType(), IntrinsicFn, AssignArgs));
+  } else {
+    Value *Args[] = {MetadataAsValue::get(Context, getRawLocation()),
+                     MetadataAsValue::get(Context, getVariable()),
+                     MetadataAsValue::get(Context, getExpression())};
+    DVI = cast<DbgVariableIntrinsic>(
+        CallInst::Create(IntrinsicFn->getFunctionType(), IntrinsicFn, Args));
+  }
   DVI->setTailCall();
   DVI->setDebugLoc(getDebugLoc());
   if (InsertBefore)
@@ -221,8 +288,30 @@ DPValue::createDebugIntrinsic(Module *M, Instruction *InsertBefore) const {
   return DVI;
 }
 
-void DPValue::handleChangedLocation(Metadata *NewLocation) {
-  resetDebugValue(NewLocation);
+Value *DPValue::getAddress() const {
+  auto *MD = getRawAddress();
+  if (auto *V = dyn_cast<ValueAsMetadata>(MD))
+    return V->getValue();
+
+  // When the value goes to null, it gets replaced by an empty MDNode.
+  assert(!cast<MDNode>(MD)->getNumOperands() && "Expected an empty MDNode");
+  return nullptr;
+}
+
+DIAssignID *DPValue::getAssignID() const {
+  return cast<DIAssignID>(DebugValues[2]);
+}
+
+void DPValue::setAssignId(DIAssignID *New) { resetDebugValue(2, New); }
+
+void DPValue::setKillAddress() {
+  resetDebugValue(
+      1, ValueAsMetadata::get(UndefValue::get(getAddress()->getType())));
+}
+
+bool DPValue::isKillAddress() const {
+  Value *Addr = getAddress();
+  return !Addr || isa<UndefValue>(Addr);
 }
 
 const BasicBlock *DPValue::getParent() const {
diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp
index 515893d079b8cb..da5d4940a6ab9b 100644
--- a/llvm/lib/IR/Metadata.cpp
+++ b/llvm/lib/IR/Metadata.cpp
@@ -152,26 +152,47 @@ DPValue *DebugValueUser::getUser() { return static_cast<DPValue *>(this); }
 const DPValue *DebugValueUser::getUser() const {
   return static_cast<const DPValue *>(this);
 }
-void DebugValueUser::handleChangedValue(Metadata *NewMD) {
-  getUser()->handleChangedLocation(NewMD);
+
+void DebugValueUser::handleChangedValue(void *Old, Metadata *New) {
+  // NOTE: We could inform the "owner" that a value has changed through
+  // getOwner, if needed.
+  auto OldMD = static_cast<Metadata **>(Old);
+  ptrdiff_t Idx = std::distance(DebugValues.begin(), OldMD);
+  resetDebugValue(Idx, New);
 }
 
-void DebugValueUser::trackDebugValue() {
-  if (DebugValue)
-    MetadataTracking::track(&DebugValue, *DebugValue, *this);
+void DebugValueUser::trackDebugValue(size_t Idx) {
+  assert(Idx < 3 && "I...
[truncated]

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 12, 2024

@llvm/pr-subscribers-debuginfo

Author: Stephen Tozer (SLTozer)

Changes

This implements the DbgAssignIntrinsic class as a variant of DPValues - unfortunately this involves increasing the size of the DebugValueUser storage by 3x, but this is necessary to enable assigns to be represented, and can be offset in a future patch by splitting DPValue into subclasses such that each variant can store only the fields it needs. This patch does not actually create DPVAssigns in any case; future patches will handle this variant in all cases where generic DPValue handling does not. This patch also does not implement tracking support for DIAssignIDs, which is necessary to find DPVAssigns that reference a given DIAssignID; that is added in a subsequent patch.


Patch is 21.60 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/77912.diff

5 Files Affected:

  • (modified) llvm/include/llvm/IR/DebugProgramInstruction.h (+52-7)
  • (modified) llvm/include/llvm/IR/Metadata.h (+42-28)
  • (modified) llvm/lib/IR/AsmWriter.cpp (+27-1)
  • (modified) llvm/lib/IR/DebugProgramInstruction.cpp (+105-16)
  • (modified) llvm/lib/IR/Metadata.cpp (+36-15)
diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h
index 8230070343e0c1..66453c1a2c1a5e 100644
--- a/llvm/include/llvm/IR/DebugProgramInstruction.h
+++ b/llvm/include/llvm/IR/DebugProgramInstruction.h
@@ -59,6 +59,7 @@ class BasicBlock;
 class MDNode;
 class Module;
 class DbgVariableIntrinsic;
+class DIAssignID;
 class DPMarker;
 class DPValue;
 class raw_ostream;
@@ -81,6 +82,7 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
   DILocalVariable *Variable;
   DIExpression *Expression;
   DebugLoc DbgLoc;
+  DIExpression *AddressExpression;
 
 public:
   void deleteInstr();
@@ -97,6 +99,7 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
   enum class LocationType {
     Declare,
     Value,
+    Assign,
 
     End, ///< Marks the end of the concrete types.
     Any, ///< To indicate all LocationTypes in searches.
@@ -117,6 +120,22 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
   /// assigning \p Location to the DV / Expr / DI variable.
   DPValue(Metadata *Location, DILocalVariable *DV, DIExpression *Expr,
           const DILocation *DI, LocationType Type = LocationType::Value);
+  DPValue(Metadata *Value, DILocalVariable *Variable, DIExpression *Expression,
+          DIAssignID *AssignID, Metadata *Address,
+          DIExpression *AddressExpression, const DILocation *DI);
+
+  static DPValue *createDPVAssign(Metadata *Value, DILocalVariable *Variable,
+                                  DIExpression *Expression,
+                                  DIAssignID *AssignID, Metadata *Address,
+                                  DIExpression *AddressExpression,
+                                  const DILocation *DI,
+                                  Instruction *InsertBefore = nullptr);
+  static DPValue *createLinkedDPVAssign(Instruction *LinkedInstr, Value *Val,
+                                        DILocalVariable *Variable,
+                                        DIExpression *Expression,
+                                        Value *Address,
+                                        DIExpression *AddressExpression,
+                                        const DILocation *DI);
 
   /// Iterator for ValueAsMetadata that internally uses direct pointer iteration
   /// over either a ValueAsMetadata* or a ValueAsMetadata**, dereferencing to the
@@ -194,7 +213,7 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
 
   /// Does this describe the address of a local variable. True for dbg.addr
   /// and dbg.declare, but not dbg.value, which describes its value.
-  bool isAddressOfVariable() const { return Type != LocationType::Value; }
+  bool isAddressOfVariable() const { return Type == LocationType::Declare; }
   LocationType getType() const { return Type; }
 
   DebugLoc getDebugLoc() const { return DbgLoc; }
@@ -207,7 +226,11 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
 
   DIExpression *getExpression() const { return Expression; }
 
-  Metadata *getRawLocation() const { return DebugValue; }
+  /// Returns the metadata operand for the first location description. i.e.,
+  /// dbg intrinsic dbg.value,declare operand and dbg.assign 1st location
+  /// operand (the "value componenet"). Note the operand (singular) may be
+  /// a DIArgList which is a list of values.
+  Metadata *getRawLocation() const { return DebugValues[0]; }
 
   /// Use of this should generally be avoided; instead,
   /// replaceVariableLocationOp and addVariableLocationOps should be used where
@@ -217,23 +240,45 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
         (isa<ValueAsMetadata>(NewLocation) || isa<DIArgList>(NewLocation) ||
          isa<MDNode>(NewLocation)) &&
         "Location for a DPValue must be either ValueAsMetadata or DIArgList");
-    resetDebugValue(NewLocation);
+    resetDebugValue(0, NewLocation);
   }
 
   /// Get the size (in bits) of the variable, or fragment of the variable that
   /// is described.
   std::optional<uint64_t> getFragmentSizeInBits() const;
 
+  /// @name DbgAssign Methods
+  /// @{
+  bool isDbgAssign() const { return getType() == LocationType::Assign; }
+
+  Value *getAddress() const;
+  Metadata *getRawAddress() const {
+    return isDbgAssign() ? DebugValues[1] : DebugValues[0];
+  }
+  Metadata *getRawAssignID() const { return DebugValues[2]; }
+  DIAssignID *getAssignID() const;
+  DIExpression *getAddressExpression() const { return AddressExpression; }
+  void setAddressExpression(DIExpression *NewExpr) {
+    AddressExpression = NewExpr;
+  }
+  void setAssignId(DIAssignID *New);
+  void setAddress(Value *V) { resetDebugValue(1, ValueAsMetadata::get(V)); }
+  /// Kill the address component.
+  void setKillAddress();
+  /// Check whether this kills the address component. This doesn't take into
+  /// account the position of the intrinsic, therefore a returned value of false
+  /// does not guarentee the address is a valid location for the variable at the
+  /// intrinsic's position in IR.
+  bool isKillAddress() const;
+
+  /// @}
+public:
   DPValue *clone() const;
   /// Convert this DPValue back into a dbg.value intrinsic.
   /// \p InsertBefore Optional position to insert this intrinsic.
   /// \returns A new dbg.value intrinsic representiung this DPValue.
   DbgVariableIntrinsic *createDebugIntrinsic(Module *M,
                                              Instruction *InsertBefore) const;
-  /// Handle changes to the location of the Value(s) that we refer to happening
-  /// "under our feet".
-  void handleChangedLocation(Metadata *NewLocation);
-
   void setMarker(DPMarker *M) { Marker = M; }
 
   DPMarker *getMarker() { return Marker; }
diff --git a/llvm/include/llvm/IR/Metadata.h b/llvm/include/llvm/IR/Metadata.h
index 4498423c4c460d..365820b95b298d 100644
--- a/llvm/include/llvm/IR/Metadata.h
+++ b/llvm/include/llvm/IR/Metadata.h
@@ -211,31 +211,39 @@ class MetadataAsValue : public Value {
 /// lookup and callback handling.
 class DebugValueUser {
 protected:
-  Metadata *DebugValue;
+  // Capacity to store 3 debug values.
+  // TODO: Not all DebugValueUser instances need all 3 elements, if we
+  // restructure the DPValue class then we can template parameterize this array
+  // size.
+  std::array<Metadata *, 3> DebugValues;
+
+  ArrayRef<Metadata *> getDebugValues() const { return DebugValues; }
 
 public:
   DPValue *getUser();
   const DPValue *getUser() const;
-  void handleChangedValue(Metadata *NewDebugValue);
+  void handleChangedValue(void *Old, Metadata *NewDebugValue);
   DebugValueUser() = default;
-  explicit DebugValueUser(Metadata *DebugValue) : DebugValue(DebugValue) {
-    trackDebugValue();
+  explicit DebugValueUser(std::array<Metadata *, 3> DebugValues)
+      : DebugValues(DebugValues) {
+    trackDebugValues();
   }
-
-  DebugValueUser(DebugValueUser &&X) : DebugValue(X.DebugValue) {
-    retrackDebugValue(X);
+  DebugValueUser(DebugValueUser &&X) {
+    DebugValues = X.DebugValues;
+    retrackDebugValues(X);
   }
-  DebugValueUser(const DebugValueUser &X) : DebugValue(X.DebugValue) {
-    trackDebugValue();
+  DebugValueUser(const DebugValueUser &X) {
+    DebugValues = X.DebugValues;
+    trackDebugValues();
   }
 
   DebugValueUser &operator=(DebugValueUser &&X) {
     if (&X == this)
       return *this;
 
-    untrackDebugValue();
-    DebugValue = X.DebugValue;
-    retrackDebugValue(X);
+    untrackDebugValues();
+    DebugValues = X.DebugValues;
+    retrackDebugValues(X);
     return *this;
   }
 
@@ -243,35 +251,41 @@ class DebugValueUser {
     if (&X == this)
       return *this;
 
-    untrackDebugValue();
-    DebugValue = X.DebugValue;
-    trackDebugValue();
+    untrackDebugValues();
+    DebugValues = X.DebugValues;
+    trackDebugValues();
     return *this;
   }
 
-  ~DebugValueUser() { untrackDebugValue(); }
+  ~DebugValueUser() { untrackDebugValues(); }
 
-  void resetDebugValue() {
-    untrackDebugValue();
-    DebugValue = nullptr;
+  void resetDebugValues() {
+    untrackDebugValues();
+    DebugValues.fill(nullptr);
   }
-  void resetDebugValue(Metadata *DebugValue) {
-    untrackDebugValue();
-    this->DebugValue = DebugValue;
-    trackDebugValue();
+
+  void resetDebugValue(size_t Idx, Metadata *DebugValue) {
+    assert(Idx < 3 && "Invalid debug value index.");
+    untrackDebugValue(Idx);
+    DebugValues[Idx] = DebugValue;
+    trackDebugValue(Idx);
   }
 
   bool operator==(const DebugValueUser &X) const {
-    return DebugValue == X.DebugValue;
+    return DebugValues == X.DebugValues;
   }
   bool operator!=(const DebugValueUser &X) const {
-    return DebugValue != X.DebugValue;
+    return DebugValues != X.DebugValues;
   }
 
 private:
-  void trackDebugValue();
-  void untrackDebugValue();
-  void retrackDebugValue(DebugValueUser &X);
+  void trackDebugValue(size_t Idx);
+  void trackDebugValues();
+
+  void untrackDebugValue(size_t Idx);
+  void untrackDebugValues();
+
+  void retrackDebugValues(DebugValueUser &X);
 };
 
 /// API for tracking metadata references through RAUW and deletion.
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
index 278cdfce411050..3c15784a0ed5eb 100644
--- a/llvm/lib/IR/AsmWriter.cpp
+++ b/llvm/lib/IR/AsmWriter.cpp
@@ -1140,6 +1140,9 @@ void SlotTracker::processFunctionMetadata(const Function &F) {
 void SlotTracker::processDPValueMetadata(const DPValue &DPV) {
   CreateMetadataSlot(DPV.getVariable());
   CreateMetadataSlot(DPV.getDebugLoc());
+  if (DPV.isDbgAssign()) {
+    CreateMetadataSlot(DPV.getAssignID());
+  }
 }
 
 void SlotTracker::processInstructionMetadata(const Instruction &I) {
@@ -4571,7 +4574,22 @@ void AssemblyWriter::printDPMarker(const DPMarker &Marker) {
 void AssemblyWriter::printDPValue(const DPValue &Value) {
   // There's no formal representation of a DPValue -- print purely as a
   // debugging aid.
-  Out << "  DPValue { ";
+  Out << "  DPValue ";
+
+  switch (Value.getType()) {
+  case DPValue::LocationType::Value:
+    Out << "value";
+    break;
+  case DPValue::LocationType::Declare:
+    Out << "declare";
+    break;
+  case DPValue::LocationType::Assign:
+    Out << "assign";
+    break;
+  default:
+    llvm_unreachable("Tried to print a DPValue with an invalid LocationType!");
+  }
+  Out << " { ";
   auto WriterCtx = getContext();
   WriteAsOperandInternal(Out, Value.getRawLocation(), WriterCtx, true);
   Out << ", ";
@@ -4579,6 +4597,14 @@ void AssemblyWriter::printDPValue(const DPValue &Value) {
   Out << ", ";
   WriteAsOperandInternal(Out, Value.getExpression(), WriterCtx, true);
   Out << ", ";
+  if (Value.isDbgAssign()) {
+    WriteAsOperandInternal(Out, Value.getAssignID(), WriterCtx, true);
+    Out << ", ";
+    WriteAsOperandInternal(Out, Value.getRawAddress(), WriterCtx, true);
+    Out << ", ";
+    WriteAsOperandInternal(Out, Value.getAddressExpression(), WriterCtx, true);
+    Out << ", ";
+  }
   WriteAsOperandInternal(Out, Value.getDebugLoc().get(), WriterCtx, true);
   Out << " marker @" << Value.getMarker();
   Out << " }";
diff --git a/llvm/lib/IR/DebugProgramInstruction.cpp b/llvm/lib/IR/DebugProgramInstruction.cpp
index 7b709a2de0335f..1c1c965b1c09d7 100644
--- a/llvm/lib/IR/DebugProgramInstruction.cpp
+++ b/llvm/lib/IR/DebugProgramInstruction.cpp
@@ -14,8 +14,9 @@
 namespace llvm {
 
 DPValue::DPValue(const DbgVariableIntrinsic *DVI)
-    : DebugValueUser(DVI->getRawLocation()), Variable(DVI->getVariable()),
-      Expression(DVI->getExpression()), DbgLoc(DVI->getDebugLoc()) {
+    : DebugValueUser({DVI->getRawLocation(), nullptr, nullptr}),
+      Variable(DVI->getVariable()), Expression(DVI->getExpression()),
+      DbgLoc(DVI->getDebugLoc()), AddressExpression(nullptr) {
   switch (DVI->getIntrinsicID()) {
   case Intrinsic::dbg_value:
     Type = LocationType::Value;
@@ -23,6 +24,15 @@ DPValue::DPValue(const DbgVariableIntrinsic *DVI)
   case Intrinsic::dbg_declare:
     Type = LocationType::Declare;
     break;
+  case Intrinsic::dbg_assign: {
+    Type = LocationType::Assign;
+    const DbgAssignIntrinsic *Assign =
+        static_cast<const DbgAssignIntrinsic *>(DVI);
+    resetDebugValue(1, Assign->getRawAddress());
+    AddressExpression = Assign->getAddressExpression();
+    setAssignId(Assign->getAssignID());
+    break;
+  }
   default:
     llvm_unreachable(
         "Trying to create a DPValue with an invalid intrinsic type!");
@@ -30,17 +40,54 @@ DPValue::DPValue(const DbgVariableIntrinsic *DVI)
 }
 
 DPValue::DPValue(const DPValue &DPV)
-    : DebugValueUser(DPV.getRawLocation()),
-      Variable(DPV.getVariable()), Expression(DPV.getExpression()),
-      DbgLoc(DPV.getDebugLoc()), Type(DPV.getType()) {}
+    : DebugValueUser(DPV.DebugValues), Variable(DPV.getVariable()),
+      Expression(DPV.getExpression()), DbgLoc(DPV.getDebugLoc()),
+      AddressExpression(DPV.AddressExpression), Type(DPV.getType()) {}
 
 DPValue::DPValue(Metadata *Location, DILocalVariable *DV, DIExpression *Expr,
                  const DILocation *DI, LocationType Type)
-    : DebugValueUser(Location), Variable(DV), Expression(Expr), DbgLoc(DI),
-      Type(Type) {}
+    : DebugValueUser({Location, nullptr, nullptr}), Variable(DV),
+      Expression(Expr), DbgLoc(DI), Type(Type) {}
+
+DPValue::DPValue(Metadata *Value, DILocalVariable *Variable,
+                 DIExpression *Expression, DIAssignID *AssignID,
+                 Metadata *Address, DIExpression *AddressExpression,
+                 const DILocation *DI)
+    : DebugValueUser({Value, Address, AssignID}), Variable(Variable),
+      Expression(Expression), DbgLoc(DI), AddressExpression(AddressExpression),
+      Type(LocationType::Assign) {}
 
 void DPValue::deleteInstr() { delete this; }
 
+DPValue *DPValue::createDPVAssign(Metadata *Value, DILocalVariable *Variable,
+                                  DIExpression *Expression,
+                                  DIAssignID *AssignID, Metadata *Address,
+                                  DIExpression *AddressExpression,
+                                  const DILocation *DI,
+                                  Instruction *InsertBefore) {
+  auto *NewDPVAssign = new DPValue(Value, Variable, Expression, AssignID,
+                                   Address, AddressExpression, DI);
+  if (InsertBefore) {
+    InsertBefore->getParent()->insertDPValueBefore(NewDPVAssign,
+                                                   InsertBefore->getIterator());
+  }
+  return NewDPVAssign;
+}
+DPValue *DPValue::createLinkedDPVAssign(Instruction *LinkedInstr, Value *Val,
+                                        DILocalVariable *Variable,
+                                        DIExpression *Expression,
+                                        Value *Address,
+                                        DIExpression *AddressExpression,
+                                        const DILocation *DI) {
+  auto *Link = LinkedInstr->getMetadata(LLVMContext::MD_DIAssignID);
+  assert(Link && "Linked instruction must have DIAssign metadata attached");
+  auto *NewDPVAssign = new DPValue(
+      ValueAsMetadata::get(Val), Variable, Expression, cast<DIAssignID>(Link),
+      ValueAsMetadata::get(Address), AddressExpression, DI);
+  LinkedInstr->getParent()->insertDPValueAfter(NewDPVAssign, LinkedInstr);
+  return NewDPVAssign;
+}
+
 iterator_range<DPValue::location_op_iterator> DPValue::location_ops() const {
   auto *MD = getRawLocation();
   // If a Value has been deleted, the "location" for this DPValue will be
@@ -96,10 +143,15 @@ static ValueAsMetadata *getAsMetadata(Value *V) {
 void DPValue::replaceVariableLocationOp(Value *OldValue, Value *NewValue,
                                         bool AllowEmpty) {
   assert(NewValue && "Values must be non-null");
+
+  bool DbgAssignAddrReplaced = isDbgAssign() && OldValue == getAddress();
+  if (DbgAssignAddrReplaced)
+    setAddress(NewValue);
+
   auto Locations = location_ops();
   auto OldIt = find(Locations, OldValue);
   if (OldIt == Locations.end()) {
-    if (AllowEmpty)
+    if (AllowEmpty || DbgAssignAddrReplaced)
       return;
     llvm_unreachable("OldValue must be a current location");
   }
@@ -190,9 +242,6 @@ DPValue::createDebugIntrinsic(Module *M, Instruction *InsertBefore) const {
          "Cannot clone from BasicBlock that is not part of a Module or "
          "DICompileUnit!");
   LLVMContext &Context = getDebugLoc()->getContext();
-  Value *Args[] = {MetadataAsValue::get(Context, getRawLocation()),
-                   MetadataAsValue::get(Context, getVariable()),
-                   MetadataAsValue::get(Context, getExpression())};
   Function *IntrinsicFn;
 
   // Work out what sort of intrinsic we're going to produce.
@@ -203,16 +252,34 @@ DPValue::createDebugIntrinsic(Module *M, Instruction *InsertBefore) const {
   case DPValue::LocationType::Value:
     IntrinsicFn = Intrinsic::getDeclaration(M, Intrinsic::dbg_value);
     break;
+  case DPValue::LocationType::Assign:
+    IntrinsicFn = Intrinsic::getDeclaration(M, Intrinsic::dbg_assign);
+    break;
   case DPValue::LocationType::End:
   case DPValue::LocationType::Any:
     llvm_unreachable("Invalid LocationType");
-    break;
   }
 
   // Create the intrinsic from this DPValue's information, optionally insert
   // into the target location.
-  DbgVariableIntrinsic *DVI = cast<DbgVariableIntrinsic>(
-      CallInst::Create(IntrinsicFn->getFunctionType(), IntrinsicFn, Args));
+  DbgVariableIntrinsic *DVI;
+  if (isDbgAssign()) {
+    Value *AssignArgs[] = {
+        MetadataAsValue::get(Context, getRawLocation()),
+        MetadataAsValue::get(Context, getVariable()),
+        MetadataAsValue::get(Context, getExpression()),
+        MetadataAsValue::get(Context, getAssignID()),
+        MetadataAsValue::get(Context, getRawAddress()),
+        MetadataAsValue::get(Context, getAddressExpression())};
+    DVI = cast<DbgVariableIntrinsic>(CallInst::Create(
+        IntrinsicFn->getFunctionType(), IntrinsicFn, AssignArgs));
+  } else {
+    Value *Args[] = {MetadataAsValue::get(Context, getRawLocation()),
+                     MetadataAsValue::get(Context, getVariable()),
+                     MetadataAsValue::get(Context, getExpression())};
+    DVI = cast<DbgVariableIntrinsic>(
+        CallInst::Create(IntrinsicFn->getFunctionType(), IntrinsicFn, Args));
+  }
   DVI->setTailCall();
   DVI->setDebugLoc(getDebugLoc());
   if (InsertBefore)
@@ -221,8 +288,30 @@ DPValue::createDebugIntrinsic(Module *M, Instruction *InsertBefore) const {
   return DVI;
 }
 
-void DPValue::handleChangedLocation(Metadata *NewLocation) {
-  resetDebugValue(NewLocation);
+Value *DPValue::getAddress() const {
+  auto *MD = getRawAddress();
+  if (auto *V = dyn_cast<ValueAsMetadata>(MD))
+    return V->getValue();
+
+  // When the value goes to null, it gets replaced by an empty MDNode.
+  assert(!cast<MDNode>(MD)->getNumOperands() && "Expected an empty MDNode");
+  return nullptr;
+}
+
+DIAssignID *DPValue::getAssignID() const {
+  return cast<DIAssignID>(DebugValues[2]);
+}
+
+void DPValue::setAssignId(DIAssignID *New) { resetDebugValue(2, New); }
+
+void DPValue::setKillAddress() {
+  resetDebugValue(
+      1, ValueAsMetadata::get(UndefValue::get(getAddress()->getType())));
+}
+
+bool DPValue::isKillAddress() const {
+  Value *Addr = getAddress();
+  return !Addr || isa<UndefValue>(Addr);
 }
 
 const BasicBlock *DPValue::getParent() const {
diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp
index 515893d079b8cb..da5d4940a6ab9b 100644
--- a/llvm/lib/IR/Metadata.cpp
+++ b/llvm/lib/IR/Metadata.cpp
@@ -152,26 +152,47 @@ DPValue *DebugValueUser::getUser() { return static_cast<DPValue *>(this); }
 const DPValue *DebugValueUser::getUser() const {
   return static_cast<const DPValue *>(this);
 }
-void DebugValueUser::handleChangedValue(Metadata *NewMD) {
-  getUser()->handleChangedLocation(NewMD);
+
+void DebugValueUser::handleChangedValue(void *Old, Metadata *New) {
+  // NOTE: We could inform the "owner" that a value has changed through
+  // getOwner, if needed.
+  auto OldMD = static_cast<Metadata **>(Old);
+  ptrdiff_t Idx = std::distance(DebugValues.begin(), OldMD);
+  resetDebugValue(Idx, New);
 }
 
-void DebugValueUser::trackDebugValue() {
-  if (DebugValue)
-    MetadataTracking::track(&DebugValue, *DebugValue, *this);
+void DebugValueUser::trackDebugValue(size_t Idx) {
+  assert(Idx < 3 && "I...
[truncated]

Copy link
Member

@jmorse jmorse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and all seems pretty reasonable. Presumably the refactoring / production of a class hierarchy is what @OCHyams is mangling right now?

DIAssignID *AssignID, Metadata *Address,
DIExpression *AddressExpression,
const DILocation *DI,
Instruction *InsertBefore = nullptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to want to be a BasicBlock::iterator so that we can take the head bit

: DebugValueUser(DPV.getRawLocation()),
Variable(DPV.getVariable()), Expression(DPV.getExpression()),
DbgLoc(DPV.getDebugLoc()), Type(DPV.getType()) {}
: DebugValueUser(DPV.DebugValues), Variable(DPV.getVariable()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit -- can we just pass DPV into the superclass, and it'll select the appropriate copy constructor? That reduces faff if the implementation of DebugValueUser changes, but it's not big deal if all of this is still to be finalised

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this part probably isn't worth changing - it's a small bit of code and it's going to be rewritten soon by the patch that separates DPValue into various base classes... that patch will have a better perspective on what the appropriate generic/rewrite-friendly approach will be, WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

Address, AddressExpression, DI);
if (InsertBefore) {
InsertBefore->getParent()->insertDPValueBefore(NewDPVAssign,
InsertBefore->getIterator());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best to have an only-iterator-taking version of this IMO, it'll force the problem out into all the call-sites, which is the correct place to consider whether the head bit is going to be set or not.

Comment on lines +159 to +161
auto OldMD = static_cast<Metadata **>(Old);
ptrdiff_t Idx = std::distance(DebugValues.begin(), OldMD);
resetDebugValue(Idx, New);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the probability that we could have duplicate metadatas, i.e. multiple entries here? I think it's idiomatic for list-heads to assign themselves with their own address when they're empty, would that show up here as same-value same-address Metadatas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean duplicate metadata as-in, the same metadata appearing multiple times in this list? If so, the answer is that we ignore that here - this function should be called once for each pointer to the metadata being replaced that's being tracked anywhere, so if the pointer appears twice in here then they'll each have their own call even if the Owner is the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, are there any alternatives to this? The method name and signature feel mismatched with what it does, in that calling this function will replace a Old/New metadata pair, but possibly not the one that the caller expected.

To be clear, the code is fine, it's the risk of someone reading this and either being confused or having an incorrect understanding that I worry about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleChangedValuePointer? In any case, the metadata RAUW mechanism is based on maintaining a map of addresses of pointers to that metadata, so if two pointers to the same MD exist in DebugValueUser, we would expect this to be called twice. We could iterate over the full user list and reset every identical pointer here, but ReplaceableMetadataImpl::replaceAllUsesWith() (which is the only thing that should call through to this) already has reference to all of those pointers, so I think this would just be duplicating work.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool; IMO best to document this in a comment then, that it's expected to operate in this way.

@SLTozer SLTozer force-pushed the gbtozers/ddd-at-3-add-dpassign branch from 8b1e44a to e2331f2 Compare January 16, 2024 13:04
@SLTozer SLTozer merged commit d499df0 into llvm:main Jan 16, 2024
3 of 4 checks passed
@SLTozer
Copy link
Contributor Author

SLTozer commented Jan 16, 2024

This patch caused an error on some buildbots due to the line ptrdiff_t Idx = std::distance(DebugValues.begin(), OldMD); - the cause being that std::distance doesn't like being given an iterator and a pointer; fixed this by adding &* prior to the first arg to convert it to a pointer, so that we're just comparing pointers.

Submitted a fix in revision da76073 that seems to have fixed the build on at least one affected bot.

justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
This implements the DbgAssignIntrinsic class as a variant of DPValues -
unfortunately this involves increasing the size of the `DebugValueUser`
storage by 3x, but this is necessary to enable assigns to be
represented, and can be offset in a future patch by splitting DPValue
into subclasses such that each variant can store only the fields it
needs. This patch does not actually create DPVAssigns in any case;
future patches will handle this variant in all cases where generic
DPValue handling does not. This patch also does not implement tracking
support for DIAssignIDs, which is necessary to find DPVAssigns that
reference a given DIAssignID; that is added in a subsequent patch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants