[RemoveDIs][NFC] Introduce DbgRecord base class [1/3] #78252
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
debuginfo
coroutines
|
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-llvm-ir Author: Orlando Cazalet-Hyams (OCHyams) ChangesPatch 1 of 3 to add llvm.dbg.label support to the RemoveDIs project. The patch Patches 1 and 2 are NFC. In this patch (1), I've left many function names as "...DPValue..." rather The DbgRecord and subclasses implement the llvm casting methods and avoids Patch is 62.48 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/78252.diff 30 Files Affected:
diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index a72b68d867f36e7..ed25cb96ae96452 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -130,10 +130,10 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
DPMarker *getNextMarker(Instruction *I);
/// Insert a DPValue into a block at the position given by \p I.
- void insertDPValueAfter(DPValue *DPV, Instruction *I);
+ void insertDPValueAfter(DbgRecord *DPV, Instruction *I);
/// Insert a DPValue into a block at the position given by \p Here.
- void insertDPValueBefore(DPValue *DPV, InstListType::iterator Here);
+ void insertDPValueBefore(DbgRecord *DPV, InstListType::iterator Here);
/// Eject any debug-info trailing at the end of a block. DPValues can
/// transiently be located "off the end" of a block if the blocks terminator
@@ -147,7 +147,7 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
/// occur: inserting into the middle of a sequence of dbg.value intrinsics
/// does not have an equivalent with DPValues.
void reinsertInstInDPValues(Instruction *I,
- std::optional<DPValue::self_iterator> Pos);
+ std::optional<DbgRecord::self_iterator> Pos);
private:
void setParent(Function *parent);
@@ -194,8 +194,9 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
friend void Instruction::moveBeforeImpl(BasicBlock &BB,
InstListType::iterator I,
bool Preserve);
- friend iterator_range<DPValue::self_iterator> Instruction::cloneDebugInfoFrom(
- const Instruction *From, std::optional<DPValue::self_iterator> FromHere,
+ friend iterator_range<DbgRecord::self_iterator>
+ Instruction::cloneDebugInfoFrom(
+ const Instruction *From, std::optional<DbgRecord::self_iterator> FromHere,
bool InsertAtHead);
/// Creates a new BasicBlock.
diff --git a/llvm/include/llvm/IR/DebugInfo.h b/llvm/include/llvm/IR/DebugInfo.h
index 36ef77f9505bc10..5b0620de61586b9 100644
--- a/llvm/include/llvm/IR/DebugInfo.h
+++ b/llvm/include/llvm/IR/DebugInfo.h
@@ -109,6 +109,8 @@ class DebugInfoFinder {
void processLocation(const Module &M, const DILocation *Loc);
// Process a DPValue, much like a DbgVariableIntrinsic.
void processDPValue(const Module &M, const DPValue &DPV);
+ /// Dispatch to DbgRecord subclasses handlers.
+ void processDbgRecord(const Module &M, const DbgRecord &DPE);
/// Process subprogram.
void processSubprogram(DISubprogram *SP);
diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h
index 8230070343e0c1d..873303143e49836 100644
--- a/llvm/include/llvm/IR/DebugProgramInstruction.h
+++ b/llvm/include/llvm/IR/DebugProgramInstruction.h
@@ -47,10 +47,12 @@
#ifndef LLVM_IR_DEBUGPROGRAMINSTRUCTION_H
#define LLVM_IR_DEBUGPROGRAMINSTRUCTION_H
-#include "llvm/ADT/ilist_node.h"
#include "llvm/ADT/ilist.h"
+#include "llvm/ADT/ilist_node.h"
#include "llvm/ADT/iterator.h"
+#include "llvm/IR/DebugInfoMetadata.h"
#include "llvm/IR/DebugLoc.h"
+#include "llvm/Support/Casting.h"
namespace llvm {
@@ -63,38 +65,92 @@ class DPMarker;
class DPValue;
class raw_ostream;
-/// Record of a variable value-assignment, aka a non instruction representation
-/// of the dbg.value intrinsic. Features various methods copied across from the
-/// Instruction class to aid ease-of-use. DPValue objects should always be
-/// linked into a DPMarker's StoredDPValues list. The marker connects a DPValue
-/// back to it's position in the BasicBlock.
+/// Base class for non-instruction debug metadata records that have positions
+/// within IR. Features various methods copied across from the Instruction
+/// class to aid ease-of-use. DbgRecords should always be linked into a
+/// DPMarker's StoredDPValues list. The marker connects a DbgRecord back to
+/// it's position in the BasicBlock.
///
-/// This class inherits from DebugValueUser to allow LLVM's metadata facilities
-/// to update our references to metadata beneath our feet.
-class DPValue : public ilist_node<DPValue>, private DebugValueUser {
- friend class DebugValueUser;
-
- // NB: there is no explicit "Value" field in this class, it's effectively the
- // DebugValueUser superclass instead. The referred to Value can either be a
- // ValueAsMetadata or a DIArgList.
+/// We need a discriminator for dyn/isa casts. In order to avoid paying for a
+/// vtable for "virtual" functions too, subclasses must add a new discriminator
+/// value (RecordKind) and cases to a few functions in the base class:
+/// deleteRecord()
+/// clone()
+/// 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 };
- DILocalVariable *Variable;
- DIExpression *Expression;
+protected:
DebugLoc DbgLoc;
+ Kind RecordKind; ///< Subclass discriminator.
public:
- void deleteInstr();
+ DbgRecord(Kind RecordKind, DebugLoc DL)
+ : DbgLoc(DL), RecordKind(RecordKind) {}
+
+ /// Methods requiring subclass implementations.
+ ///@{
+ void deleteRecord();
+ DbgRecord *clone() const;
+ void print(raw_ostream &O, bool IsForDebug = false) const;
+ void print(raw_ostream &O, ModuleSlotTracker &MST, bool IsForDebug) const;
+ ///@}
+
+ Kind getRecordKind() const { return RecordKind; }
+
+ void setMarker(DPMarker *M) { Marker = M; }
+
+ DPMarker *getMarker() { return Marker; }
+ const DPMarker *getMarker() const { return Marker; }
+
+ BasicBlock *getBlock();
+ const BasicBlock *getBlock() const;
+
+ Function *getFunction();
+ const Function *getFunction() const;
+
+ Module *getModule();
+ const Module *getModule() const;
+
+ LLVMContext &getContext();
+ const LLVMContext &getContext() const;
const BasicBlock *getParent() const;
BasicBlock *getParent();
- void dump() const;
+
void removeFromParent();
void eraseFromParent();
- using self_iterator = simple_ilist<DPValue>::iterator;
- using const_self_iterator = simple_ilist<DPValue>::const_iterator;
+ DebugLoc getDebugLoc() const { return DbgLoc; }
+ void setDebugLoc(DebugLoc Loc) { DbgLoc = std::move(Loc); }
+
+ void dump() const;
- enum class LocationType {
+ using self_iterator = simple_ilist<DbgRecord>::iterator;
+ using const_self_iterator = simple_ilist<DbgRecord>::const_iterator;
+
+protected:
+ /// Similarly to Value, we avoid paying the cost of a vtable
+ /// by protecting the dtor and having deleteRecord dispatch
+ /// cleanup.
+ /// Use deleteRecord to delete a generic record.
+ ~DbgRecord() = default;
+};
+
+/// Record of a variable value-assignment, aka a non instruction representation
+/// of the dbg.value intrinsic.
+///
+/// This class inherits from DebugValueUser to allow LLVM's metadata facilities
+/// to update our references to metadata beneath our feet.
+class DPValue : public DbgRecord, protected DebugValueUser {
+ friend class DebugValueUser;
+
+public:
+ enum class LocationType : uint8_t {
Declare,
Value,
@@ -104,11 +160,17 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
/// Classification of the debug-info record that this DPValue represents.
/// Essentially, "is this a dbg.value or dbg.declare?". dbg.declares are not
/// currently supported, but it would be trivial to do so.
+ /// FIXME: We could use spare padding bits from DbgRecord for this.
LocationType Type;
- /// Marker that this DPValue is linked into.
- DPMarker *Marker = nullptr;
+ // NB: there is no explicit "Value" field in this class, it's effectively the
+ // DebugValueUser superclass instead. The referred to Value can either be a
+ // ValueAsMetadata or a DIArgList.
+ DILocalVariable *Variable;
+ DIExpression *Expression;
+
+public:
/// Create a new DPValue representing the intrinsic \p DVI, for example the
/// assignment represented by a dbg.value.
DPValue(const DbgVariableIntrinsic *DVI);
@@ -197,9 +259,6 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
bool isAddressOfVariable() const { return Type != LocationType::Value; }
LocationType getType() const { return Type; }
- DebugLoc getDebugLoc() const { return DbgLoc; }
- void setDebugLoc(DebugLoc Loc) { DbgLoc = std::move(Loc); }
-
void setKillLocation();
bool isKillLocation() const;
@@ -230,40 +289,37 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
/// \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; }
- const DPMarker *getMarker() const { return Marker; }
-
- BasicBlock *getBlock();
- const BasicBlock *getBlock() const;
-
- Function *getFunction();
- const Function *getFunction() const;
-
- Module *getModule();
- const Module *getModule() const;
-
- LLVMContext &getContext();
- const LLVMContext &getContext() const;
-
void print(raw_ostream &O, bool IsForDebug = false) const;
void print(raw_ostream &ROS, ModuleSlotTracker &MST, bool IsForDebug) const;
+
+ /// Filter the DbgRecord range to DPValue types only and downcast.
+ static inline auto
+ filter(iterator_range<simple_ilist<DbgRecord>::iterator> R) {
+ return map_range(
+ make_filter_range(R, [](DbgRecord &E) { return isa<DPValue>(E); }),
+ [](DbgRecord &E) { return std::ref(cast<DPValue>(E)); });
+ }
+
+ /// Support type inquiry through isa, cast, and dyn_cast.
+ static bool classof(const DbgRecord *E) {
+ return E->getRecordKind() == ValueKind;
+ }
};
/// Per-instruction record of debug-info. If an Instruction is the position of
/// some debugging information, it points at a DPMarker storing that info. Each
/// marker points back at the instruction that owns it. Various utilities are
-/// provided for manipulating the DPValues contained within this marker.
+/// provided for manipulating the DbgRecords contained within this marker.
///
-/// This class has a rough surface area, because it's needed to preserve the one
-/// arefact that we can't yet eliminate from the intrinsic / dbg.value
-/// debug-info design: the order of DPValues/records is significant, and
-/// duplicates can exist. Thus, if one has a run of debug-info records such as:
+/// This class has a rough surface area, because it's needed to preserve the
+/// one arefact that we can't yet eliminate from the intrinsic / dbg.value
+/// debug-info design: the order of records is significant, and duplicates can
+/// exist. Thus, if one has a run of debug-info records such as:
/// dbg.value(...
/// %foo = barinst
/// dbg.value(...
@@ -283,12 +339,11 @@ class DPMarker {
/// operations that move a marker from one instruction to another.
Instruction *MarkedInstr = nullptr;
- /// List of DPValues, each recording a single variable assignment, the
- /// equivalent of a dbg.value intrinsic. There is a one-to-one relationship
- /// between each dbg.value in a block and each DPValue once the
- /// representation has been converted, and the ordering of DPValues is
- /// meaningful in the same was a dbg.values.
- simple_ilist<DPValue> StoredDPValues;
+ /// List of DbgRecords, the non-instruction equivalent of llvm.dbg.*
+ /// intrinsics. There is a one-to-one relationship between each debug
+ /// intrinsic in a block and each DbgRecord once the representation has been
+ /// converted, and the ordering is meaningful in the same way.
+ simple_ilist<DbgRecord> StoredDPValues;
bool empty() const { return StoredDPValues.empty(); }
const BasicBlock *getParent() const;
@@ -308,34 +363,34 @@ class DPMarker {
void print(raw_ostream &ROS, ModuleSlotTracker &MST, bool IsForDebug) const;
/// Produce a range over all the DPValues in this Marker.
- iterator_range<simple_ilist<DPValue>::iterator> getDbgValueRange();
+ iterator_range<simple_ilist<DbgRecord>::iterator> getDbgValueRange();
/// 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);
/// Transfer the DPValues in \p Range from \p Src into this DPMarker. If
/// \p InsertAtHead is true, place them before existing DPValues, otherwise
// afterwards.
- void absorbDebugValues(iterator_range<DPValue::self_iterator> Range,
+ void absorbDebugValues(iterator_range<DbgRecord::self_iterator> Range,
DPMarker &Src, bool InsertAtHead);
/// 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);
+ void insertDPValue(DbgRecord *New, bool InsertAtHead);
/// Clone all DPMarkers from \p From into this marker. There are numerous
/// options to customise the source/destination, due to gnarliness, see class
/// comment.
/// \p FromHere If non-null, copy from FromHere to the end of From's DPValues
/// \p InsertAtHead Place the cloned DPValues at the start of StoredDPValues
/// \returns Range over all the newly cloned DPValues
- iterator_range<simple_ilist<DPValue>::iterator>
+ iterator_range<simple_ilist<DbgRecord>::iterator>
cloneDebugInfoFrom(DPMarker *From,
- std::optional<simple_ilist<DPValue>::iterator> FromHere,
+ std::optional<simple_ilist<DbgRecord>::iterator> FromHere,
bool InsertAtHead = false);
/// Erase all DPValues in this DPMarker.
- void dropDPValues();
- /// Erase a single DPValue from this marker. In an ideal future, we would
+ void dropDbgValues();
+ /// Erase a single DbgRecord from this marker. In an ideal future, we would
/// never erase an assignment in this way, but it's the equivalent to
- /// erasing a dbg.value from a block.
- void dropOneDPValue(DPValue *DPV);
+ /// erasing a debug intrinsic from a block.
+ void dropOneDbgValue(DbgRecord *DPE);
/// We generally act like all llvm Instructions have a range of DPValues
/// attached to them, but in reality sometimes we don't allocate the DPMarker
@@ -345,8 +400,10 @@ class DPMarker {
/// DPValue in that range, but they should be using the Official (TM) API for
/// that.
static DPMarker EmptyDPMarker;
- static iterator_range<simple_ilist<DPValue>::iterator> getEmptyDPValueRange(){
- return make_range(EmptyDPMarker.StoredDPValues.end(), EmptyDPMarker.StoredDPValues.end());
+ static iterator_range<simple_ilist<DbgRecord>::iterator>
+ getEmptyDPValueRange() {
+ return make_range(EmptyDPMarker.StoredDPValues.end(),
+ EmptyDPMarker.StoredDPValues.end());
}
};
diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index 0211b5076131ce6..736c774ca3122fe 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -35,6 +35,7 @@ class MDNode;
class Module;
struct AAMDNodes;
class DPMarker;
+class DbgRecord;
template <> struct ilist_alloc_traits<Instruction> {
static inline void deleteNode(Instruction *V);
@@ -70,18 +71,18 @@ class Instruction : public User,
/// \p InsertAtHead Whether the cloned DPValues should be placed at the end
/// or the beginning of existing DPValues attached to this.
/// \returns A range over the newly cloned DPValues.
- iterator_range<simple_ilist<DPValue>::iterator> cloneDebugInfoFrom(
+ iterator_range<simple_ilist<DbgRecord>::iterator> cloneDebugInfoFrom(
const Instruction *From,
- std::optional<simple_ilist<DPValue>::iterator> FromHere = std::nullopt,
+ std::optional<simple_ilist<DbgRecord>::iterator> FromHere = std::nullopt,
bool InsertAtHead = false);
/// Return a range over the DPValues attached to this instruction.
- iterator_range<simple_ilist<DPValue>::iterator> getDbgValueRange() const;
+ iterator_range<simple_ilist<DbgRecord>::iterator> getDbgValueRange() const;
/// Return an iterator to the position of the "Next" DPValue after this
/// instruction, or std::nullopt. This is the position to pass to
/// BasicBlock::reinsertInstInDPValues when re-inserting an instruction.
- std::optional<simple_ilist<DPValue>::iterator> getDbgReinsertionPosition();
+ std::optional<simple_ilist<DbgRecord>::iterator> getDbgReinsertionPosition();
/// Returns true if any DPValues are attached to this instruction.
bool hasDbgValues() const;
@@ -90,7 +91,7 @@ class Instruction : public User,
void dropDbgValues();
/// Erase a single DPValue \p I that is attached to this instruction.
- void dropOneDbgValue(DPValue *I);
+ void dropOneDbgValue(DbgRecord *I);
/// Handle the debug-info implications of this instruction being removed. Any
/// attached DPValues need to "fall" down onto the next instruction.
diff --git a/llvm/include/llvm/IR/Metadata.h b/llvm/include/llvm/IR/Metadata.h
index 4498423c4c460d9..bb5d479b26ce492 100644
--- a/llvm/include/llvm/IR/Metadata.h
+++ b/llvm/include/llvm/IR/Metadata.h
@@ -44,6 +44,7 @@ class Module;
class ModuleSlotTracker;
class raw_ostream;
class DPValue;
+class DbgRecord;
template <typename T> class StringMapEntry;
template <typename ValueTy> class StringMapEntryStorage;
class Type;
diff --git a/llvm/include/llvm/Transforms/Utils/ValueMapper.h b/llvm/include/llvm/Transforms/Utils/ValueMapper.h
index e1f2796d97ceb18..dd183bc04f0c249 100644
--- a/llvm/include/llvm/Transforms/Utils/ValueMapper.h
+++ b/llvm/include/llvm/Transforms/Utils/ValueMapper.h
@@ -22,7 +22,8 @@
namespace llvm {
class Constant;
-class DPValue;
+class DIBuilder;
+class DbgRecord;
class Function;
class GlobalVariable;
class Instruction;
@@ -33,7 +34,7 @@ class Type;
class Value;
using ValueToValueMapTy = ValueMap<const Value *, WeakTrackingVH>;
-using DPValueIterator = simple_ilist<DPValue>::iterator;
+using DbgRecordIterator = simple_ilist<DbgRecord>::iterator;
/// This is a class that can be implemented by clients to remap types when
/// cloning constants and instructions.
@@ -180,7 +181,7 @@ class ValueMapper {
void remapInstruction(Instruction &I);
void remapDPValue(Module *M, DPValue &V);
- void remapDPValueRange(Module *M, iterator_range<DPValueIterator> Range);
+ void remapDPValueRange(Module *M, iterator_range<DbgRecordIterator> Range);
void remapFunction(Function &F);
void remapGlobalObjectMetadata(GlobalObject &GO);
@@ -275,7 +276,8 @@ inline void RemapDPValue(Module *M, DPValue *V, ValueToValueMapTy &VM,
}
/// Remap the Values used in the DPValue \a V using the value map \a VM.
-inline void RemapDPValueRange(Module *M, iterator_range<DPValueIterator> Range,
+inline void RemapDPValueRange(Module *M,
+ iterator_range<DbgRecordIterator> Range,
ValueToValueMapTy &VM, RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr) {
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 5bd4c6b067d796b..9e310d85110a897 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -8400,7 +8400,7 @@ bool CodeGenPrepare::fixupDbgValue(Instruction *I) {
bool CodeGenPrepare::fixupDPValuesOnInst(Instruction &I) {
bool AnyChange = false;
- for (DPValue &DPV : I.getDbgValueRange())
+ for (DPValue &DPV : DPValue::filter(I.getDbgValueRange()))
AnyChange |= fixupDPValue(DPV);
return AnyChange;
}
@@ -8512,7 +8512,8 @@ bool CodeGenPrepare::placeDbgValues(Function &F) {
// If this isn't a dbg.value, process any attached DPValue records
// attached to this instruction.
- for (DPValue &DPV : llvm::make_early_inc_range(Insn.getDbgValueRange())) {
+ for (DPValue &DPV : llvm::...
[truncated]
|
|
@llvm/pr-subscribers-debuginfo Author: Orlando Cazalet-Hyams (OCHyams) ChangesPatch 1 of 3 to add llvm.dbg.label support to the RemoveDIs project. The patch Patches 1 and 2 are NFC. In this patch (1), I've left many function names as "...DPValue..." rather The DbgRecord and subclasses implement the llvm casting methods and avoids Patch is 62.48 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/78252.diff 30 Files Affected:
diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index a72b68d867f36e7..ed25cb96ae96452 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -130,10 +130,10 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
DPMarker *getNextMarker(Instruction *I);
/// Insert a DPValue into a block at the position given by \p I.
- void insertDPValueAfter(DPValue *DPV, Instruction *I);
+ void insertDPValueAfter(DbgRecord *DPV, Instruction *I);
/// Insert a DPValue into a block at the position given by \p Here.
- void insertDPValueBefore(DPValue *DPV, InstListType::iterator Here);
+ void insertDPValueBefore(DbgRecord *DPV, InstListType::iterator Here);
/// Eject any debug-info trailing at the end of a block. DPValues can
/// transiently be located "off the end" of a block if the blocks terminator
@@ -147,7 +147,7 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
/// occur: inserting into the middle of a sequence of dbg.value intrinsics
/// does not have an equivalent with DPValues.
void reinsertInstInDPValues(Instruction *I,
- std::optional<DPValue::self_iterator> Pos);
+ std::optional<DbgRecord::self_iterator> Pos);
private:
void setParent(Function *parent);
@@ -194,8 +194,9 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
friend void Instruction::moveBeforeImpl(BasicBlock &BB,
InstListType::iterator I,
bool Preserve);
- friend iterator_range<DPValue::self_iterator> Instruction::cloneDebugInfoFrom(
- const Instruction *From, std::optional<DPValue::self_iterator> FromHere,
+ friend iterator_range<DbgRecord::self_iterator>
+ Instruction::cloneDebugInfoFrom(
+ const Instruction *From, std::optional<DbgRecord::self_iterator> FromHere,
bool InsertAtHead);
/// Creates a new BasicBlock.
diff --git a/llvm/include/llvm/IR/DebugInfo.h b/llvm/include/llvm/IR/DebugInfo.h
index 36ef77f9505bc10..5b0620de61586b9 100644
--- a/llvm/include/llvm/IR/DebugInfo.h
+++ b/llvm/include/llvm/IR/DebugInfo.h
@@ -109,6 +109,8 @@ class DebugInfoFinder {
void processLocation(const Module &M, const DILocation *Loc);
// Process a DPValue, much like a DbgVariableIntrinsic.
void processDPValue(const Module &M, const DPValue &DPV);
+ /// Dispatch to DbgRecord subclasses handlers.
+ void processDbgRecord(const Module &M, const DbgRecord &DPE);
/// Process subprogram.
void processSubprogram(DISubprogram *SP);
diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h
index 8230070343e0c1d..873303143e49836 100644
--- a/llvm/include/llvm/IR/DebugProgramInstruction.h
+++ b/llvm/include/llvm/IR/DebugProgramInstruction.h
@@ -47,10 +47,12 @@
#ifndef LLVM_IR_DEBUGPROGRAMINSTRUCTION_H
#define LLVM_IR_DEBUGPROGRAMINSTRUCTION_H
-#include "llvm/ADT/ilist_node.h"
#include "llvm/ADT/ilist.h"
+#include "llvm/ADT/ilist_node.h"
#include "llvm/ADT/iterator.h"
+#include "llvm/IR/DebugInfoMetadata.h"
#include "llvm/IR/DebugLoc.h"
+#include "llvm/Support/Casting.h"
namespace llvm {
@@ -63,38 +65,92 @@ class DPMarker;
class DPValue;
class raw_ostream;
-/// Record of a variable value-assignment, aka a non instruction representation
-/// of the dbg.value intrinsic. Features various methods copied across from the
-/// Instruction class to aid ease-of-use. DPValue objects should always be
-/// linked into a DPMarker's StoredDPValues list. The marker connects a DPValue
-/// back to it's position in the BasicBlock.
+/// Base class for non-instruction debug metadata records that have positions
+/// within IR. Features various methods copied across from the Instruction
+/// class to aid ease-of-use. DbgRecords should always be linked into a
+/// DPMarker's StoredDPValues list. The marker connects a DbgRecord back to
+/// it's position in the BasicBlock.
///
-/// This class inherits from DebugValueUser to allow LLVM's metadata facilities
-/// to update our references to metadata beneath our feet.
-class DPValue : public ilist_node<DPValue>, private DebugValueUser {
- friend class DebugValueUser;
-
- // NB: there is no explicit "Value" field in this class, it's effectively the
- // DebugValueUser superclass instead. The referred to Value can either be a
- // ValueAsMetadata or a DIArgList.
+/// We need a discriminator for dyn/isa casts. In order to avoid paying for a
+/// vtable for "virtual" functions too, subclasses must add a new discriminator
+/// value (RecordKind) and cases to a few functions in the base class:
+/// deleteRecord()
+/// clone()
+/// 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 };
- DILocalVariable *Variable;
- DIExpression *Expression;
+protected:
DebugLoc DbgLoc;
+ Kind RecordKind; ///< Subclass discriminator.
public:
- void deleteInstr();
+ DbgRecord(Kind RecordKind, DebugLoc DL)
+ : DbgLoc(DL), RecordKind(RecordKind) {}
+
+ /// Methods requiring subclass implementations.
+ ///@{
+ void deleteRecord();
+ DbgRecord *clone() const;
+ void print(raw_ostream &O, bool IsForDebug = false) const;
+ void print(raw_ostream &O, ModuleSlotTracker &MST, bool IsForDebug) const;
+ ///@}
+
+ Kind getRecordKind() const { return RecordKind; }
+
+ void setMarker(DPMarker *M) { Marker = M; }
+
+ DPMarker *getMarker() { return Marker; }
+ const DPMarker *getMarker() const { return Marker; }
+
+ BasicBlock *getBlock();
+ const BasicBlock *getBlock() const;
+
+ Function *getFunction();
+ const Function *getFunction() const;
+
+ Module *getModule();
+ const Module *getModule() const;
+
+ LLVMContext &getContext();
+ const LLVMContext &getContext() const;
const BasicBlock *getParent() const;
BasicBlock *getParent();
- void dump() const;
+
void removeFromParent();
void eraseFromParent();
- using self_iterator = simple_ilist<DPValue>::iterator;
- using const_self_iterator = simple_ilist<DPValue>::const_iterator;
+ DebugLoc getDebugLoc() const { return DbgLoc; }
+ void setDebugLoc(DebugLoc Loc) { DbgLoc = std::move(Loc); }
+
+ void dump() const;
- enum class LocationType {
+ using self_iterator = simple_ilist<DbgRecord>::iterator;
+ using const_self_iterator = simple_ilist<DbgRecord>::const_iterator;
+
+protected:
+ /// Similarly to Value, we avoid paying the cost of a vtable
+ /// by protecting the dtor and having deleteRecord dispatch
+ /// cleanup.
+ /// Use deleteRecord to delete a generic record.
+ ~DbgRecord() = default;
+};
+
+/// Record of a variable value-assignment, aka a non instruction representation
+/// of the dbg.value intrinsic.
+///
+/// This class inherits from DebugValueUser to allow LLVM's metadata facilities
+/// to update our references to metadata beneath our feet.
+class DPValue : public DbgRecord, protected DebugValueUser {
+ friend class DebugValueUser;
+
+public:
+ enum class LocationType : uint8_t {
Declare,
Value,
@@ -104,11 +160,17 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
/// Classification of the debug-info record that this DPValue represents.
/// Essentially, "is this a dbg.value or dbg.declare?". dbg.declares are not
/// currently supported, but it would be trivial to do so.
+ /// FIXME: We could use spare padding bits from DbgRecord for this.
LocationType Type;
- /// Marker that this DPValue is linked into.
- DPMarker *Marker = nullptr;
+ // NB: there is no explicit "Value" field in this class, it's effectively the
+ // DebugValueUser superclass instead. The referred to Value can either be a
+ // ValueAsMetadata or a DIArgList.
+ DILocalVariable *Variable;
+ DIExpression *Expression;
+
+public:
/// Create a new DPValue representing the intrinsic \p DVI, for example the
/// assignment represented by a dbg.value.
DPValue(const DbgVariableIntrinsic *DVI);
@@ -197,9 +259,6 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
bool isAddressOfVariable() const { return Type != LocationType::Value; }
LocationType getType() const { return Type; }
- DebugLoc getDebugLoc() const { return DbgLoc; }
- void setDebugLoc(DebugLoc Loc) { DbgLoc = std::move(Loc); }
-
void setKillLocation();
bool isKillLocation() const;
@@ -230,40 +289,37 @@ class DPValue : public ilist_node<DPValue>, private DebugValueUser {
/// \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; }
- const DPMarker *getMarker() const { return Marker; }
-
- BasicBlock *getBlock();
- const BasicBlock *getBlock() const;
-
- Function *getFunction();
- const Function *getFunction() const;
-
- Module *getModule();
- const Module *getModule() const;
-
- LLVMContext &getContext();
- const LLVMContext &getContext() const;
-
void print(raw_ostream &O, bool IsForDebug = false) const;
void print(raw_ostream &ROS, ModuleSlotTracker &MST, bool IsForDebug) const;
+
+ /// Filter the DbgRecord range to DPValue types only and downcast.
+ static inline auto
+ filter(iterator_range<simple_ilist<DbgRecord>::iterator> R) {
+ return map_range(
+ make_filter_range(R, [](DbgRecord &E) { return isa<DPValue>(E); }),
+ [](DbgRecord &E) { return std::ref(cast<DPValue>(E)); });
+ }
+
+ /// Support type inquiry through isa, cast, and dyn_cast.
+ static bool classof(const DbgRecord *E) {
+ return E->getRecordKind() == ValueKind;
+ }
};
/// Per-instruction record of debug-info. If an Instruction is the position of
/// some debugging information, it points at a DPMarker storing that info. Each
/// marker points back at the instruction that owns it. Various utilities are
-/// provided for manipulating the DPValues contained within this marker.
+/// provided for manipulating the DbgRecords contained within this marker.
///
-/// This class has a rough surface area, because it's needed to preserve the one
-/// arefact that we can't yet eliminate from the intrinsic / dbg.value
-/// debug-info design: the order of DPValues/records is significant, and
-/// duplicates can exist. Thus, if one has a run of debug-info records such as:
+/// This class has a rough surface area, because it's needed to preserve the
+/// one arefact that we can't yet eliminate from the intrinsic / dbg.value
+/// debug-info design: the order of records is significant, and duplicates can
+/// exist. Thus, if one has a run of debug-info records such as:
/// dbg.value(...
/// %foo = barinst
/// dbg.value(...
@@ -283,12 +339,11 @@ class DPMarker {
/// operations that move a marker from one instruction to another.
Instruction *MarkedInstr = nullptr;
- /// List of DPValues, each recording a single variable assignment, the
- /// equivalent of a dbg.value intrinsic. There is a one-to-one relationship
- /// between each dbg.value in a block and each DPValue once the
- /// representation has been converted, and the ordering of DPValues is
- /// meaningful in the same was a dbg.values.
- simple_ilist<DPValue> StoredDPValues;
+ /// List of DbgRecords, the non-instruction equivalent of llvm.dbg.*
+ /// intrinsics. There is a one-to-one relationship between each debug
+ /// intrinsic in a block and each DbgRecord once the representation has been
+ /// converted, and the ordering is meaningful in the same way.
+ simple_ilist<DbgRecord> StoredDPValues;
bool empty() const { return StoredDPValues.empty(); }
const BasicBlock *getParent() const;
@@ -308,34 +363,34 @@ class DPMarker {
void print(raw_ostream &ROS, ModuleSlotTracker &MST, bool IsForDebug) const;
/// Produce a range over all the DPValues in this Marker.
- iterator_range<simple_ilist<DPValue>::iterator> getDbgValueRange();
+ iterator_range<simple_ilist<DbgRecord>::iterator> getDbgValueRange();
/// 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);
/// Transfer the DPValues in \p Range from \p Src into this DPMarker. If
/// \p InsertAtHead is true, place them before existing DPValues, otherwise
// afterwards.
- void absorbDebugValues(iterator_range<DPValue::self_iterator> Range,
+ void absorbDebugValues(iterator_range<DbgRecord::self_iterator> Range,
DPMarker &Src, bool InsertAtHead);
/// 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);
+ void insertDPValue(DbgRecord *New, bool InsertAtHead);
/// Clone all DPMarkers from \p From into this marker. There are numerous
/// options to customise the source/destination, due to gnarliness, see class
/// comment.
/// \p FromHere If non-null, copy from FromHere to the end of From's DPValues
/// \p InsertAtHead Place the cloned DPValues at the start of StoredDPValues
/// \returns Range over all the newly cloned DPValues
- iterator_range<simple_ilist<DPValue>::iterator>
+ iterator_range<simple_ilist<DbgRecord>::iterator>
cloneDebugInfoFrom(DPMarker *From,
- std::optional<simple_ilist<DPValue>::iterator> FromHere,
+ std::optional<simple_ilist<DbgRecord>::iterator> FromHere,
bool InsertAtHead = false);
/// Erase all DPValues in this DPMarker.
- void dropDPValues();
- /// Erase a single DPValue from this marker. In an ideal future, we would
+ void dropDbgValues();
+ /// Erase a single DbgRecord from this marker. In an ideal future, we would
/// never erase an assignment in this way, but it's the equivalent to
- /// erasing a dbg.value from a block.
- void dropOneDPValue(DPValue *DPV);
+ /// erasing a debug intrinsic from a block.
+ void dropOneDbgValue(DbgRecord *DPE);
/// We generally act like all llvm Instructions have a range of DPValues
/// attached to them, but in reality sometimes we don't allocate the DPMarker
@@ -345,8 +400,10 @@ class DPMarker {
/// DPValue in that range, but they should be using the Official (TM) API for
/// that.
static DPMarker EmptyDPMarker;
- static iterator_range<simple_ilist<DPValue>::iterator> getEmptyDPValueRange(){
- return make_range(EmptyDPMarker.StoredDPValues.end(), EmptyDPMarker.StoredDPValues.end());
+ static iterator_range<simple_ilist<DbgRecord>::iterator>
+ getEmptyDPValueRange() {
+ return make_range(EmptyDPMarker.StoredDPValues.end(),
+ EmptyDPMarker.StoredDPValues.end());
}
};
diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index 0211b5076131ce6..736c774ca3122fe 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -35,6 +35,7 @@ class MDNode;
class Module;
struct AAMDNodes;
class DPMarker;
+class DbgRecord;
template <> struct ilist_alloc_traits<Instruction> {
static inline void deleteNode(Instruction *V);
@@ -70,18 +71,18 @@ class Instruction : public User,
/// \p InsertAtHead Whether the cloned DPValues should be placed at the end
/// or the beginning of existing DPValues attached to this.
/// \returns A range over the newly cloned DPValues.
- iterator_range<simple_ilist<DPValue>::iterator> cloneDebugInfoFrom(
+ iterator_range<simple_ilist<DbgRecord>::iterator> cloneDebugInfoFrom(
const Instruction *From,
- std::optional<simple_ilist<DPValue>::iterator> FromHere = std::nullopt,
+ std::optional<simple_ilist<DbgRecord>::iterator> FromHere = std::nullopt,
bool InsertAtHead = false);
/// Return a range over the DPValues attached to this instruction.
- iterator_range<simple_ilist<DPValue>::iterator> getDbgValueRange() const;
+ iterator_range<simple_ilist<DbgRecord>::iterator> getDbgValueRange() const;
/// Return an iterator to the position of the "Next" DPValue after this
/// instruction, or std::nullopt. This is the position to pass to
/// BasicBlock::reinsertInstInDPValues when re-inserting an instruction.
- std::optional<simple_ilist<DPValue>::iterator> getDbgReinsertionPosition();
+ std::optional<simple_ilist<DbgRecord>::iterator> getDbgReinsertionPosition();
/// Returns true if any DPValues are attached to this instruction.
bool hasDbgValues() const;
@@ -90,7 +91,7 @@ class Instruction : public User,
void dropDbgValues();
/// Erase a single DPValue \p I that is attached to this instruction.
- void dropOneDbgValue(DPValue *I);
+ void dropOneDbgValue(DbgRecord *I);
/// Handle the debug-info implications of this instruction being removed. Any
/// attached DPValues need to "fall" down onto the next instruction.
diff --git a/llvm/include/llvm/IR/Metadata.h b/llvm/include/llvm/IR/Metadata.h
index 4498423c4c460d9..bb5d479b26ce492 100644
--- a/llvm/include/llvm/IR/Metadata.h
+++ b/llvm/include/llvm/IR/Metadata.h
@@ -44,6 +44,7 @@ class Module;
class ModuleSlotTracker;
class raw_ostream;
class DPValue;
+class DbgRecord;
template <typename T> class StringMapEntry;
template <typename ValueTy> class StringMapEntryStorage;
class Type;
diff --git a/llvm/include/llvm/Transforms/Utils/ValueMapper.h b/llvm/include/llvm/Transforms/Utils/ValueMapper.h
index e1f2796d97ceb18..dd183bc04f0c249 100644
--- a/llvm/include/llvm/Transforms/Utils/ValueMapper.h
+++ b/llvm/include/llvm/Transforms/Utils/ValueMapper.h
@@ -22,7 +22,8 @@
namespace llvm {
class Constant;
-class DPValue;
+class DIBuilder;
+class DbgRecord;
class Function;
class GlobalVariable;
class Instruction;
@@ -33,7 +34,7 @@ class Type;
class Value;
using ValueToValueMapTy = ValueMap<const Value *, WeakTrackingVH>;
-using DPValueIterator = simple_ilist<DPValue>::iterator;
+using DbgRecordIterator = simple_ilist<DbgRecord>::iterator;
/// This is a class that can be implemented by clients to remap types when
/// cloning constants and instructions.
@@ -180,7 +181,7 @@ class ValueMapper {
void remapInstruction(Instruction &I);
void remapDPValue(Module *M, DPValue &V);
- void remapDPValueRange(Module *M, iterator_range<DPValueIterator> Range);
+ void remapDPValueRange(Module *M, iterator_range<DbgRecordIterator> Range);
void remapFunction(Function &F);
void remapGlobalObjectMetadata(GlobalObject &GO);
@@ -275,7 +276,8 @@ inline void RemapDPValue(Module *M, DPValue *V, ValueToValueMapTy &VM,
}
/// Remap the Values used in the DPValue \a V using the value map \a VM.
-inline void RemapDPValueRange(Module *M, iterator_range<DPValueIterator> Range,
+inline void RemapDPValueRange(Module *M,
+ iterator_range<DbgRecordIterator> Range,
ValueToValueMapTy &VM, RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr) {
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 5bd4c6b067d796b..9e310d85110a897 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -8400,7 +8400,7 @@ bool CodeGenPrepare::fixupDbgValue(Instruction *I) {
bool CodeGenPrepare::fixupDPValuesOnInst(Instruction &I) {
bool AnyChange = false;
- for (DPValue &DPV : I.getDbgValueRange())
+ for (DPValue &DPV : DPValue::filter(I.getDbgValueRange()))
AnyChange |= fixupDPValue(DPV);
return AnyChange;
}
@@ -8512,7 +8512,8 @@ bool CodeGenPrepare::placeDbgValues(Function &F) {
// If this isn't a dbg.value, process any attached DPValue records
// attached to this instruction.
- for (DPValue &DPV : llvm::make_early_inc_range(Insn.getDbgValueRange())) {
+ for (DPValue &DPV : llvm::...
[truncated]
|
You can test this locally with the following command:git-clang-format --diff 5bd374df3e89df701ea8c02e5704911935660d1e ad0dfa5f50c122937ecfc01dbfaebee115814fb0 -- llvm/include/llvm/IR/BasicBlock.h llvm/include/llvm/IR/DebugInfo.h llvm/include/llvm/IR/DebugProgramInstruction.h llvm/include/llvm/IR/Instruction.h llvm/include/llvm/Transforms/Utils/ValueMapper.h llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp llvm/lib/CodeGen/CodeGenPrepare.cpp llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp llvm/lib/CodeGen/SelectionDAG/FastISel.cpp llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp llvm/lib/IR/AsmWriter.cpp llvm/lib/IR/BasicBlock.cpp llvm/lib/IR/DebugInfo.cpp llvm/lib/IR/DebugProgramInstruction.cpp llvm/lib/IR/Instruction.cpp llvm/lib/Transforms/Coroutines/CoroFrame.cpp llvm/lib/Transforms/Coroutines/CoroSplit.cpp llvm/lib/Transforms/IPO/MergeFunctions.cpp llvm/lib/Transforms/InstCombine/InstructionCombining.cpp llvm/lib/Transforms/Scalar/ADCE.cpp llvm/lib/Transforms/Scalar/JumpThreading.cpp llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp llvm/lib/Transforms/Utils/BasicBlockUtils.cpp llvm/lib/Transforms/Utils/CodeExtractor.cpp llvm/lib/Transforms/Utils/InlineFunction.cpp llvm/lib/Transforms/Utils/Local.cpp llvm/lib/Transforms/Utils/LoopRotationUtils.cpp llvm/lib/Transforms/Utils/LoopUtils.cpp llvm/lib/Transforms/Utils/MemoryTaggingSupport.cpp llvm/lib/Transforms/Utils/SimplifyCFG.cpp llvm/lib/Transforms/Utils/ValueMapper.cpp llvm/tools/llvm-reduce/DeltaManager.cpp llvm/tools/llvm-reduce/deltas/ReduceDPValues.cpp llvm/tools/llvm-reduce/deltas/ReduceDPValues.h llvm/unittests/IR/BasicBlockDbgInfoTest.cpp llvm/unittests/IR/DebugInfoTest.cpp llvm/unittests/IR/ValueTest.cppView the diff from clang-format here.diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
index 251485a403..8ce8c3fa45 100644
--- a/llvm/lib/IR/AsmWriter.cpp
+++ b/llvm/lib/IR/AsmWriter.cpp
@@ -5155,7 +5155,10 @@ void DPMarker::dump() const { print(dbgs(), /*IsForDebug=*/true); dbgs() << '\n'
// Value::dump - allow easy printing of Values from the debugger.
LLVM_DUMP_METHOD
-void DbgRecord::dump() const { print(dbgs(), /*IsForDebug=*/true); dbgs() << '\n'; }
+void DbgRecord::dump() const {
+ print(dbgs(), /*IsForDebug=*/true);
+ dbgs() << '\n';
+}
// Type::dump - allow easy printing of Types from the debugger.
LLVM_DUMP_METHOD
diff --git a/llvm/tools/llvm-reduce/DeltaManager.cpp b/llvm/tools/llvm-reduce/DeltaManager.cpp
index fa42920ee9..746ad5d5db 100644
--- a/llvm/tools/llvm-reduce/DeltaManager.cpp
+++ b/llvm/tools/llvm-reduce/DeltaManager.cpp
@@ -112,7 +112,7 @@ static cl::list<std::string>
DELTA_PASS("atomic-ordering", reduceAtomicOrderingDeltaPass) \
DELTA_PASS("syncscopes", reduceAtomicSyncScopesDeltaPass) \
DELTA_PASS("instruction-flags", reduceInstructionFlagsDeltaPass) \
-} while (false)
+ } while (false)
#define DELTA_PASSES_MIR \
do { \
|
jmorse
approved these changes
Jan 16, 2024
There was a problem hiding this comment.
LGTM with some minor things inline to fiddle with. Am I right in thinking that there are effectively two levels of "kinds" to deal with -- kinds of DbgRecords and kinds of DPValues? And is that going to change in the later patches -- reason being that there's potential for people being confused by this, plus there's precedent in the rest of LLVM to have a single enum for each hierachy and use isa/dyn_cast/etc to distinguish them.
Starting to change all the names of all of these things, now that we've got a better handle on what's going on, sounds like a good idea.
| /// erasing a dbg.value from a block. | ||
| void dropOneDPValue(DPValue *DPV); | ||
| /// erasing a debug intrinsic from a block. | ||
| void dropOneDbgValue(DbgRecord *DPE); |
There was a problem hiding this comment.
DPE -> DPEntity? (One imagines this gets revised in the future?)
There was a problem hiding this comment.
Yeah that's a hangover from the original base class name DPEntity. I endeavoured to fixup names like this in the follow up patch. Not sure I caught this one. I'll fix instances of DPE -> DR in this patch.
llvm/lib/IR/BasicBlock.cpp
Outdated
| InstList.insert(Inst.getIterator(), | ||
| DPV->createDebugIntrinsic(getModule(), nullptr)); | ||
| else | ||
| llvm_unreachable("unsupported entity kind"); |
There was a problem hiding this comment.
IMO this wants some kind of indication that we're dealing with DbgRecords / DPValues in the error message, if/when this pops up for developers, they're going to immediately want to know "it's debug-info at fault", which might not be obvious otherwise.
llvm/lib/IR/DebugInfo.cpp
Outdated
| @@ -89,10 +89,11 @@ static void findDbgIntrinsics(SmallVectorImpl<IntrinsicT *> &Result, Value *V, | |||
| if (!DPValues) | |||
| continue; | |||
| DIArgList *DI = cast<DIArgList>(AL); | |||
| for (DPValue *DPV : DI->getAllDPValueUsers()) | |||
| for (DPValue *DPV : DI->getAllDPValueUsers()) { | |||
| delete cast<DPValue>(this); | ||
| break; | ||
| default: | ||
| llvm_unreachable("unsupported record kind"); |
There was a problem hiding this comment.
IMO these unreachable messages want a less generic text, something to indicate it's debug-info going wrong
Comment on lines
385
to
576
| for (DbgRecord &DPE : Range) | ||
| DPE.setMarker(this); |
There was a problem hiding this comment.
DPEs -> DbgRecs, or will DPEs become clearer in the future?
| DbgRecord(Kind RecordKind, DebugLoc DL) | ||
| : DbgLoc(DL), RecordKind(RecordKind) {} | ||
|
|
||
| /// Methods requiring subclass implementations. |
There was a problem hiding this comment.
First time I read this I guessed it meant "subclasses must define these methods", but what it really means is that there's a switch in the implementation on the kind? Some kind of rewording would be appreciated, not really sure how though.
There was a problem hiding this comment.
wdyt of "Methods that dispatch to subclass implementations. These need to be manually updated when a new subclass is added."?
OCHyams
force-pushed
the
ddd/labels-upstream-1
branch
from
541bd2b
to
d92252b
Compare
OCHyams
commented
Feb 20, 2024
There was a problem hiding this comment.
Thanks @jmorse - I've now rebased and addressed comments.
After recent offline discussion we concluded that the 2nd patch,
- Rename DPValue -> DbgVariableRecord, DPMarker -> DbgMarker."
would be better off delayed - it can land with incoming documentation updates that @SLTozer is working on (unfortunately the existing patch became stale very quickly). That way we get functionality in an tested more quickly (and reverts are easier).
That means the next patch in this series will add the DbgLabel class (which I will rename DPLabel to keep things consistent until full rename is complete).
Does that plan sound acceptable?
| DbgRecord(Kind RecordKind, DebugLoc DL) | ||
| : DbgLoc(DL), RecordKind(RecordKind) {} | ||
|
|
||
| /// Methods requiring subclass implementations. |
| /// erasing a dbg.value from a block. | ||
| void dropOneDPValue(DPValue *DPV); | ||
| /// erasing a debug intrinsic from a block. | ||
| void dropOneDbgValue(DbgRecord *DPE); |
llvm/lib/IR/BasicBlock.cpp
Outdated
| InstList.insert(Inst.getIterator(), | ||
| DPV->createDebugIntrinsic(getModule(), nullptr)); | ||
| else | ||
| llvm_unreachable("unsupported entity kind"); |
| delete cast<DPValue>(this); | ||
| break; | ||
| default: | ||
| llvm_unreachable("unsupported record kind"); |
Comment on lines
385
to
576
| for (DbgRecord &DPE : Range) | ||
| DPE.setMarker(this); |
Yes there's a |
SLTozer
reviewed
Feb 20, 2024
| @@ -50,9 +50,11 @@ | |||
| #include "llvm/ADT/ilist.h" | |||
| #include "llvm/ADT/ilist_node.h" | |||
| #include "llvm/ADT/iterator.h" | |||
| #include "llvm/IR/DebugInfoMetadata.h" | |||
There was a problem hiding this comment.
Is it possible to avoid this include? Adding this header here causes a significant increase in compile times in my experience.
llvm/include/llvm/IR/Metadata.h
Outdated
| @@ -44,6 +44,7 @@ class Module; | |||
| class ModuleSlotTracker; | |||
| class raw_ostream; | |||
| class DPValue; | |||
| class DbgRecord; | |||
SLTozer
approved these changes
Feb 20, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Patch 1 of 4 to add llvm.dbg.label support to the RemoveDIs project. The patch
stack adds a new base class, and takes this opportunity to rename the RemoveDIs
classes to something more meaningful.
Patches 1, 2, and 3 are NFC. The 2nd patch is rather large (but mechanical).
In this patch (1), I've left many function names as "...DPValue..." rather
than renaming them to "...DbgRecord...", instead shunting that work all into
the second patch.
The DbgRecord and subclasses implement the llvm casting methods and avoids
adding a vtable by having a couple of dispatching methods on the base class
(including for cleanup).
The subsequent patches are currently pull requests on my fork, I'll move them over as the dependencies are merged. Here are the links, in case the overall shape is needed for this review.
2 - OCHyams#3
3 - OCHyams#4
4 - OCHyams#5