[RemoveDIs][DebugInfo][NFC] Add Instruction and convenience functions to DPValue #77896
Conversation
|
@llvm/pr-subscribers-debuginfo @llvm/pr-subscribers-llvm-ir Author: Stephen Tozer (SLTozer) ChangesThis patch adds a set of functions to the DPValue class that conveniently perform some common operations, and some that replicate existing functions on Full diff: https://github.com/llvm/llvm-project/pull/77896.diff 2 Files Affected:
diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h
index 8230070343e0c1..0001b1a7ca0f06 100644
--- a/llvm/include/llvm/IR/DebugProgramInstruction.h
+++ b/llvm/include/llvm/IR/DebugProgramInstruction.h
@@ -91,6 +91,9 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
void removeFromParent();
void eraseFromParent();
+ DPValue *getNextNode() { return &*(++getIterator()); }
+ DPValue *getPrevNode() { return &*(--getIterator()); }
+
using self_iterator = simple_ilist<DPValue>::iterator;
using const_self_iterator = simple_ilist<DPValue>::const_iterator;
@@ -118,6 +121,16 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
DPValue(Metadata *Location, DILocalVariable *DV, DIExpression *Expr,
const DILocation *DI, LocationType Type = LocationType::Value);
+ static DPValue *createDPValue(Value *Location, DILocalVariable *DV,
+ DIExpression *Expr, const DILocation *DI,
+ Instruction *InsertBefore = nullptr);
+ static DPValue *createDPValue(Value *Location, DILocalVariable *DV,
+ DIExpression *Expr, const DILocation *DI,
+ DPValue *InsertBefore);
+ static DPValue *createDPDeclare(Value *Address, DILocalVariable *DV,
+ DIExpression *Expr, const DILocation *DI,
+ Instruction *InsertBefore = nullptr);
+
/// Iterator for ValueAsMetadata that internally uses direct pointer iteration
/// over either a ValueAsMetadata* or a ValueAsMetadata**, dereferencing to the
/// ValueAsMetadata .
@@ -166,6 +179,9 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
}
};
+ bool isDbgDeclare() { return Type == LocationType::Declare; }
+ bool isDbgValue() { return Type == LocationType::Value; }
+
/// Get the locations corresponding to the variable referenced by the debug
/// info intrinsic. Depending on the intrinsic, this could be the
/// variable's value or its address.
@@ -209,6 +225,10 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
Metadata *getRawLocation() const { return DebugValue; }
+ Value *getValue(unsigned OpIdx = 0) const {
+ return getVariableLocationOp(OpIdx);
+ }
+
/// Use of this should generally be avoided; instead,
/// replaceVariableLocationOp and addVariableLocationOps should be used where
/// possible to avoid creating invalid state.
@@ -224,6 +244,19 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
/// is described.
std::optional<uint64_t> getFragmentSizeInBits() const;
+ bool isEquivalentTo(const DPValue &Other) {
+ return std::tie(Type, DebugValue, Variable, Expression, DbgLoc) ==
+ std::tie(Other.Type, Other.DebugValue, Other.Variable,
+ Other.Expression, Other.DbgLoc);
+ }
+ // Matches the definition of the Instruction version, equivalent to above but
+ // without checking DbgLoc.
+ bool isIdenticalToWhenDefined(const DPValue &Other) {
+ return std::tie(Type, DebugValue, Variable, Expression) ==
+ std::tie(Other.Type, Other.DebugValue, Other.Variable,
+ Other.Expression);
+ }
+
DPValue *clone() const;
/// Convert this DPValue back into a dbg.value intrinsic.
/// \p InsertBefore Optional position to insert this intrinsic.
@@ -251,6 +284,13 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
LLVMContext &getContext();
const LLVMContext &getContext() const;
+ /// Insert this DPValue prior to \p InsertBefore. Must not be called if this
+ /// is already contained in a DPMarker.
+ void insertBefore(DPValue *InsertBefore);
+ void insertAfter(DPValue *InsertAfter);
+ void moveBefore(DPValue *MoveBefore);
+ void moveAfter(DPValue *MoveAfter);
+
void print(raw_ostream &O, bool IsForDebug = false) const;
void print(raw_ostream &ROS, ModuleSlotTracker &MST, bool IsForDebug) const;
};
@@ -309,6 +349,8 @@ class DPMarker {
/// Produce a range over all the DPValues in this Marker.
iterator_range<simple_ilist<DPValue>::iterator> getDbgValueRange();
+ iterator_range<simple_ilist<DPValue>::const_iterator>
+ getDbgValueRange() const;
/// Transfer any DPValues from \p Src into this DPMarker. If \p InsertAtHead
/// is true, place them before existing DPValues, otherwise afterwards.
void absorbDebugValues(DPMarker &Src, bool InsertAtHead);
@@ -320,6 +362,10 @@ class DPMarker {
/// Insert a DPValue into this DPMarker, at the end of the list. If
/// \p InsertAtHead is true, at the start.
void insertDPValue(DPValue *New, bool InsertAtHead);
+ /// Insert a DPValue prior to a DPValue contained within this marker.
+ void insertDPValue(DPValue *New, DPValue *InsertBefore);
+ /// Insert a DPValue after a DPValue contained within this marker.
+ void insertDPValueAfter(DPValue *New, DPValue *InsertAfter);
/// Clone all DPMarkers from \p From into this marker. There are numerous
/// options to customise the source/destination, due to gnarliness, see class
/// comment.
diff --git a/llvm/lib/IR/DebugProgramInstruction.cpp b/llvm/lib/IR/DebugProgramInstruction.cpp
index 7b709a2de0335f..8a16dd1acd9d6c 100644
--- a/llvm/lib/IR/DebugProgramInstruction.cpp
+++ b/llvm/lib/IR/DebugProgramInstruction.cpp
@@ -41,6 +41,36 @@ DPValue::DPValue(Metadata *Location, DILocalVariable *DV, DIExpression *Expr,
void DPValue::deleteInstr() { delete this; }
+DPValue *DPValue::createDPValue(Value *Location, DILocalVariable *DV,
+ DIExpression *Expr, const DILocation *DI,
+ Instruction *InsertBefore) {
+ auto *NewDPValue = new DPValue(ValueAsMetadata::get(Location), DV, Expr, DI,
+ LocationType::Value);
+ if (InsertBefore)
+ InsertBefore->getParent()->insertDPValueBefore(NewDPValue,
+ InsertBefore->getIterator());
+ return NewDPValue;
+}
+DPValue *DPValue::createDPValue(Value *Location, DILocalVariable *DV,
+ DIExpression *Expr, const DILocation *DI,
+ DPValue *InsertBefore) {
+ auto *NewDPValue = new DPValue(ValueAsMetadata::get(Location), DV, Expr, DI,
+ LocationType::Value);
+ if (InsertBefore)
+ NewDPValue->insertBefore(InsertBefore);
+ return NewDPValue;
+}
+DPValue *DPValue::createDPDeclare(Value *Address, DILocalVariable *DV,
+ DIExpression *Expr, const DILocation *DI,
+ Instruction *InsertBefore) {
+ auto *NewDPDeclare = new DPValue(ValueAsMetadata::get(Address), DV, Expr, DI,
+ LocationType::Declare);
+ if (InsertBefore)
+ InsertBefore->getParent()->insertDPValueBefore(NewDPDeclare,
+ InsertBefore->getIterator());
+ return NewDPDeclare;
+}
+
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
@@ -249,6 +279,35 @@ const LLVMContext &DPValue::getContext() const {
return getBlock()->getContext();
}
+void DPValue::insertBefore(DPValue *InsertBefore) {
+ assert(!getMarker() &&
+ "Cannot insert a DPValue that is already has a DPMarker!");
+ assert(InsertBefore->getMarker() &&
+ "Cannot insert a DPValue before a DPValue that does not have a "
+ "DPMarker!");
+ InsertBefore->getMarker()->insertDPValue(this, InsertBefore);
+}
+void DPValue::insertAfter(DPValue *InsertAfter) {
+ assert(!getMarker() &&
+ "Cannot insert a DPValue that is already has a DPMarker!");
+ assert(InsertAfter->getMarker() &&
+ "Cannot insert a DPValue after a DPValue that does not have a "
+ "DPMarker!");
+ InsertAfter->getMarker()->insertDPValueAfter(this, InsertAfter);
+}
+void DPValue::moveBefore(DPValue *MoveBefore) {
+ assert(getMarker() &&
+ "Canot move a DPValue that does not currently have a DPMarker!");
+ removeFromParent();
+ insertBefore(MoveBefore);
+}
+void DPValue::moveAfter(DPValue *MoveAfter) {
+ assert(getMarker() &&
+ "Canot move a DPValue that does not currently have a DPMarker!");
+ removeFromParent();
+ insertAfter(MoveAfter);
+}
+
///////////////////////////////////////////////////////////////////////////////
// An empty, global, DPMarker for the purpose of describing empty ranges of
@@ -313,9 +372,14 @@ void DPMarker::eraseFromParent() {
iterator_range<DPValue::self_iterator> DPMarker::getDbgValueRange() {
return make_range(StoredDPValues.begin(), StoredDPValues.end());
}
+iterator_range<DPValue::const_self_iterator>
+DPMarker::getDbgValueRange() const {
+ return make_range(StoredDPValues.begin(), StoredDPValues.end());
+}
void DPValue::removeFromParent() {
getMarker()->StoredDPValues.erase(getIterator());
+ Marker = nullptr;
}
void DPValue::eraseFromParent() {
@@ -328,6 +392,18 @@ void DPMarker::insertDPValue(DPValue *New, bool InsertAtHead) {
StoredDPValues.insert(It, *New);
New->setMarker(this);
}
+void DPMarker::insertDPValue(DPValue *New, DPValue *InsertBefore) {
+ assert(InsertBefore->getMarker() == this &&
+ "DPValue 'InsertBefore' must be contained in this DPMarker!");
+ StoredDPValues.insert(InsertBefore->getIterator(), *New);
+ New->setMarker(this);
+}
+void DPMarker::insertDPValueAfter(DPValue *New, DPValue *InsertAfter) {
+ assert(InsertAfter->getMarker() == this &&
+ "DPValue 'InsertAfter' must be contained in this DPMarker!");
+ StoredDPValues.insert(++(InsertAfter->getIterator()), *New);
+ New->setMarker(this);
+}
void DPMarker::absorbDebugValues(DPMarker &Src, bool InsertAtHead) {
auto It = InsertAtHead ? StoredDPValues.begin() : StoredDPValues.end();
|
There was a problem hiding this comment.
LGTM, with some inline suggestions. I think this is overall good, however seeing getNextNode etc associated with DPValues makes me twitch, it's a pavlovian response as getNextNode is almost always associated with a gnochange issue in LLVM-IR. I don't think we'll keep this interface forever though, so this should be fine.
| DPValue *getNextNode() { return &*(++getIterator()); } | ||
| DPValue *getPrevNode() { return &*(--getIterator()); } |
There was a problem hiding this comment.
I'd strongly suggest std::next or std::prev rather than increment operators -- that avoid there being any idea of mutating state.
| InsertBefore->getParent()->insertDPValueBefore(NewDPValue, | ||
| InsertBefore->getIterator()); |
There was a problem hiding this comment.
Do you think it's worth having an iterator-taking version too, to match the equivalent insertBefore with BasicBlock::iterator? We don't do before-or-after debuginfo shenanigans here, but that might be necessary if you're looking to have source-equivalence between dbg.value and DPValue mode implementations of code.
There was a problem hiding this comment.
Perhaps - there might be performance concerns here though: allowing BasicBlock::iterator to be passed as an argument here would require DebugProgramInstruction.h to #include "llvm/IR/Instruction.h", which might impact performance (including DebugInfoMetadata.h here caused a +2.10% compile time increase entirely by itself!). I agree that we should probably take a BasicBlock::iterator if possible though; would you consider it acceptable to simply remove the Instruction *InsertBefore version (the DPValue* InsertBefore is the one that is actually needed for these patches, this overload is optional) and add an iterator-taking version when/if it's confirmed to work, or some other workaround is established?
There was a problem hiding this comment.
You miiiggghht be able to predeclare it as SymbolTableList<Instruction, ilist_iterator_bits<true>>::iterator; without needing a body definition for Instruction.
Removing and adding when necessary seems alright -- hopefully we can avoid it being necessary.
There was a problem hiding this comment.
I tried that first, but Instruction.h defines a template specialization for Instruction, so unfortunately the predeclaration doesn't work.
| @@ -328,6 +392,18 @@ void DPMarker::insertDPValue(DPValue *New, bool InsertAtHead) { | |||
| StoredDPValues.insert(It, *New); | |||
| New->setMarker(this); | |||
| } | |||
There was a problem hiding this comment.
Nit, whitespace between functions desired.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
… to DPValue (llvm#77896) This patch adds a set of functions to the DPValue class that conveniently perform some common operations, and some that replicate existing functions on `DbgVariableIntrinsic` and its subclasses.
This patch adds a set of functions to the DPValue class that conveniently perform some common operations, and some that replicate existing functions on
DbgVariableIntrinsicand its subclasses. These functions aren't used in this patch, but will be used in patches to come (added in this patch to make review easier - they all perform trivial tasks or exactly replicate the behaviour of existing functions).