Skip to content

[Utils] Identity map module-level debug info on first use in CloneFunction* #118627

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Jan 24, 2025
Merged

[Utils] Identity map module-level debug info on first use in CloneFunction* #118627

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions llvm/include/llvm/Transforms/Utils/Cloning.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,8 @@ void CloneFunctionAttributesInto(Function *NewFunc, const Function *OldFunc,
void CloneFunctionMetadataInto(Function &NewFunc, const Function &OldFunc,
ValueToValueMapTy &VMap, RemapFlags RemapFlag,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr);
ValueMaterializer *Materializer = nullptr,
const MetadataSetTy *IdentityMD = nullptr);

/// Clone OldFunc's body into NewFunc.
void CloneFunctionBodyInto(Function &NewFunc, const Function &OldFunc,
Expand All @@ -202,7 +203,8 @@ void CloneFunctionBodyInto(Function &NewFunc, const Function &OldFunc,
const char *NameSuffix = "",
ClonedCodeInfo *CodeInfo = nullptr,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr);
ValueMaterializer *Materializer = nullptr,
const MetadataSetTy *IdentityMD = nullptr);

void CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc,
const Instruction *StartingInst,
Expand Down Expand Up @@ -242,13 +244,24 @@ DISubprogram *CollectDebugInfoForCloning(const Function &F,
CloneFunctionChangeType Changes,
DebugInfoFinder &DIFinder);

/// Build a map of debug info to use during Metadata cloning.
/// Returns true if cloning would need module level changes and false if there
/// would only be local changes.
bool BuildDebugInfoMDMap(DenseMap<const Metadata *, TrackingMDRef> &MD,
CloneFunctionChangeType Changes,
DebugInfoFinder &DIFinder,
DISubprogram *SPClonedWithinModule);
/// Based on \p Changes and \p DIFinder return debug info that needs to be
/// identity mapped during Metadata cloning.
///
/// NOTE: Such \a MetadataSetTy can be used by \a CloneFunction* to directly
/// specify metadata that should be identity mapped (and hence not cloned). The
/// metadata will be identity mapped in \a ValueToValueMapTy on first use. There
/// are several reasons for doing it this way rather than eagerly identity
/// mapping metadata nodes in a \a ValueMap:
/// 1. Mapping metadata is not cheap, particularly because of tracking.
/// 2. When cloning a Function we identity map lots of global module-level
/// metadata to avoid cloning it, while only a fraction of it is actually
/// used by the function. Mapping on first use is a lot faster for modules
/// with meaningful amount of debug info.
/// 3. Eagerly identity mapping metadata makes it harder to cache module-level
/// data (e.g. a set of metadata nodes in a \a DICompileUnit).
MetadataSetTy FindDebugInfoToIdentityMap(CloneFunctionChangeType Changes,
DebugInfoFinder &DIFinder,
DISubprogram *SPClonedWithinModule);

/// This class captures the data input to the InlineFunction call, and records
/// the auxiliary results produced by it.
Expand Down
60 changes: 41 additions & 19 deletions llvm/include/llvm/Transforms/Utils/ValueMapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#define LLVM_TRANSFORMS_UTILS_VALUEMAPPER_H

#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/simple_ilist.h"
#include "llvm/IR/ValueHandle.h"
#include "llvm/IR/ValueMap.h"
Expand All @@ -35,6 +36,7 @@ class Value;

using ValueToValueMapTy = ValueMap<const Value *, WeakTrackingVH>;
using DbgRecordIterator = simple_ilist<DbgRecord>::iterator;
using MetadataSetTy = SmallPtrSet<const Metadata *, 16>;

/// This is a class that can be implemented by clients to remap types when
/// cloning constants and instructions.
Expand Down Expand Up @@ -112,7 +114,7 @@ inline RemapFlags operator|(RemapFlags LHS, RemapFlags RHS) {
///
/// A shared context used for mapping and remapping of Value and Metadata
/// instances using \a ValueToValueMapTy, \a RemapFlags, \a
/// ValueMapTypeRemapper, and \a ValueMaterializer.
/// ValueMapTypeRemapper, \a ValueMaterializer, and \a IdentityMD.
///
/// There are a number of top-level entry points:
/// - \a mapValue() (and \a mapConstant());
Expand All @@ -136,6 +138,9 @@ inline RemapFlags operator|(RemapFlags LHS, RemapFlags RHS) {
/// alternate \a ValueToValueMapTy and \a ValueMaterializer and returns a ID to
/// pass into the schedule*() functions.
///
/// If an \a IdentityMD set is optionally provided, \a Metadata inside this set
/// will be mapped onto itself in \a VM on first use.
///
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not place a comment about Cloning functions inside of the value mapper header; this comment is mostly relevant to "Cloning" users of this abstraction. We would better express this by moving this comment to Cloning.h.

In ValueMapper.h, we should add a brief comment talking about when users of this class should provide an IdentityMD and what it does. In other words, here we should provide guidance for people looking to use ValueMapper. Something short and descriptive like:

If an IdentityMD set is optionally provided, all Metadata inside this set will be mapped onto itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fair! I moved the NOTE to Cloning.h/FindDebugInfoToIdentityMap. I don't think there's a single best place in Cloning.h to attach this NOTE to, but FindDebugInfoToIdentityMap feels like a reasonable one.

/// TODO: lib/Linker really doesn't need the \a ValueHandle in the \a
/// ValueToValueMapTy. We should template \a ValueMapper (and its
/// implementation classes), and explicitly instantiate on two concrete
Expand All @@ -152,7 +157,8 @@ class ValueMapper {
public:
ValueMapper(ValueToValueMapTy &VM, RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr);
ValueMaterializer *Materializer = nullptr,
const MetadataSetTy *IdentityMD = nullptr);
ValueMapper(ValueMapper &&) = delete;
ValueMapper(const ValueMapper &) = delete;
ValueMapper &operator=(ValueMapper &&) = delete;
Expand Down Expand Up @@ -218,8 +224,10 @@ class ValueMapper {
inline Value *MapValue(const Value *V, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr) {
return ValueMapper(VM, Flags, TypeMapper, Materializer).mapValue(*V);
ValueMaterializer *Materializer = nullptr,
const MetadataSetTy *IdentityMD = nullptr) {
return ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.mapValue(*V);
}

/// Lookup or compute a mapping for a piece of metadata.
Expand All @@ -231,7 +239,9 @@ inline Value *MapValue(const Value *V, ValueToValueMapTy &VM,
/// \c MD.
/// 3. Else if \c MD is a \a ConstantAsMetadata, call \a MapValue() and
/// re-wrap its return (returning nullptr on nullptr).
/// 4. Else, \c MD is an \a MDNode. These are remapped, along with their
/// 4. Else if \c MD is in \c IdentityMD then add an identity mapping for it
/// and return it.
/// 5. Else, \c MD is an \a MDNode. These are remapped, along with their
/// transitive operands. Distinct nodes are duplicated or moved depending
/// on \a RF_MoveDistinctNodes. Uniqued nodes are remapped like constants.
///
Expand All @@ -240,16 +250,20 @@ inline Value *MapValue(const Value *V, ValueToValueMapTy &VM,
inline Metadata *MapMetadata(const Metadata *MD, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr) {
return ValueMapper(VM, Flags, TypeMapper, Materializer).mapMetadata(*MD);
ValueMaterializer *Materializer = nullptr,
const MetadataSetTy *IdentityMD = nullptr) {
return ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.mapMetadata(*MD);
}

/// Version of MapMetadata with type safety for MDNode.
inline MDNode *MapMetadata(const MDNode *MD, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr) {
return ValueMapper(VM, Flags, TypeMapper, Materializer).mapMDNode(*MD);
ValueMaterializer *Materializer = nullptr,
const MetadataSetTy *IdentityMD = nullptr) {
return ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.mapMDNode(*MD);
}

/// Convert the instruction operands from referencing the current values into
Expand All @@ -263,17 +277,21 @@ inline MDNode *MapMetadata(const MDNode *MD, ValueToValueMapTy &VM,
inline void RemapInstruction(Instruction *I, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer).remapInstruction(*I);
ValueMaterializer *Materializer = nullptr,
const MetadataSetTy *IdentityMD = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.remapInstruction(*I);
}

/// Remap the Values used in the DbgRecord \a DR using the value map \a
/// VM.
inline void RemapDbgRecord(Module *M, DbgRecord *DR, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer).remapDbgRecord(M, *DR);
ValueMaterializer *Materializer = nullptr,
const MetadataSetTy *IdentityMD = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.remapDbgRecord(M, *DR);
}

/// Remap the Values used in the DbgRecords \a Range using the value map \a
Expand All @@ -283,8 +301,9 @@ inline void RemapDbgRecordRange(Module *M,
ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer)
ValueMaterializer *Materializer = nullptr,
const MetadataSetTy *IdentityMD = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.remapDbgRecordRange(M, Range);
}

Expand All @@ -297,16 +316,19 @@ inline void RemapDbgRecordRange(Module *M,
inline void RemapFunction(Function &F, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer).remapFunction(F);
ValueMaterializer *Materializer = nullptr,
const MetadataSetTy *IdentityMD = nullptr) {
ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD).remapFunction(F);
}

/// Version of MapValue with type safety for Constant.
inline Constant *MapValue(const Constant *V, ValueToValueMapTy &VM,
RemapFlags Flags = RF_None,
ValueMapTypeRemapper *TypeMapper = nullptr,
ValueMaterializer *Materializer = nullptr) {
return ValueMapper(VM, Flags, TypeMapper, Materializer).mapConstant(*V);
ValueMaterializer *Materializer = nullptr,
const MetadataSetTy *IdentityMD = nullptr) {
return ValueMapper(VM, Flags, TypeMapper, Materializer, IdentityMD)
.mapConstant(*V);
}

} // end namespace llvm
Expand Down
62 changes: 29 additions & 33 deletions llvm/lib/Transforms/Utils/CloneFunction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,61 +152,52 @@ DISubprogram *llvm::CollectDebugInfoForCloning(const Function &F,
return SPClonedWithinModule;
}

bool llvm::BuildDebugInfoMDMap(DenseMap<const Metadata *, TrackingMDRef> &MD,
CloneFunctionChangeType Changes,
DebugInfoFinder &DIFinder,
DISubprogram *SPClonedWithinModule) {
bool ModuleLevelChanges = Changes > CloneFunctionChangeType::LocalChangesOnly;
MetadataSetTy
llvm::FindDebugInfoToIdentityMap(CloneFunctionChangeType Changes,
DebugInfoFinder &DIFinder,
DISubprogram *SPClonedWithinModule) {
MetadataSetTy MD;

if (Changes < CloneFunctionChangeType::DifferentModule &&
DIFinder.subprogram_count() > 0) {
// Turn on module-level changes, since we need to clone (some of) the
// debug info metadata.
//
// FIXME: Metadata effectively owned by a function should be made
// local, and only that local metadata should be cloned.
ModuleLevelChanges = true;

auto mapToSelfIfNew = [&MD](MDNode *N) {
// Avoid clobbering an existing mapping.
(void)MD.try_emplace(N, N);
};

// Avoid cloning types, compile units, and (other) subprograms.
for (DISubprogram *ISP : DIFinder.subprograms()) {
if (ISP != SPClonedWithinModule)
mapToSelfIfNew(ISP);
MD.insert(ISP);
}

// If a subprogram isn't going to be cloned skip its lexical blocks as well.
for (DIScope *S : DIFinder.scopes()) {
auto *LScope = dyn_cast<DILocalScope>(S);
if (LScope && LScope->getSubprogram() != SPClonedWithinModule)
mapToSelfIfNew(S);
MD.insert(S);
}

for (DICompileUnit *CU : DIFinder.compile_units())
mapToSelfIfNew(CU);
MD.insert(CU);

for (DIType *Type : DIFinder.types())
mapToSelfIfNew(Type);
MD.insert(Type);
} else {
assert(!SPClonedWithinModule &&
"Subprogram should be in DIFinder->subprogram_count()...");
}

return ModuleLevelChanges;
return MD;
}

void llvm::CloneFunctionMetadataInto(Function &NewFunc, const Function &OldFunc,
ValueToValueMapTy &VMap,
RemapFlags RemapFlag,
ValueMapTypeRemapper *TypeMapper,
ValueMaterializer *Materializer) {
ValueMaterializer *Materializer,
const MetadataSetTy *IdentityMD) {
SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
OldFunc.getAllMetadata(MDs);
for (auto MD : MDs) {
NewFunc.addMetadata(MD.first, *MapMetadata(MD.second, VMap, RemapFlag,
TypeMapper, Materializer));
NewFunc.addMetadata(MD.first,
*MapMetadata(MD.second, VMap, RemapFlag, TypeMapper,
Materializer, IdentityMD));
}
}

Expand All @@ -216,7 +207,8 @@ void llvm::CloneFunctionBodyInto(Function &NewFunc, const Function &OldFunc,
const char *NameSuffix,
ClonedCodeInfo *CodeInfo,
ValueMapTypeRemapper *TypeMapper,
ValueMaterializer *Materializer) {
ValueMaterializer *Materializer,
const MetadataSetTy *IdentityMD) {
if (OldFunc.isDeclaration())
return;

Expand Down Expand Up @@ -258,9 +250,10 @@ void llvm::CloneFunctionBodyInto(Function &NewFunc, const Function &OldFunc,
// Loop over all instructions, fixing each one as we find it, and any
// attached debug-info records.
for (Instruction &II : *BB) {
RemapInstruction(&II, VMap, RemapFlag, TypeMapper, Materializer);
RemapInstruction(&II, VMap, RemapFlag, TypeMapper, Materializer,
IdentityMD);
RemapDbgRecordRange(II.getModule(), II.getDbgRecordRange(), VMap,
RemapFlag, TypeMapper, Materializer);
RemapFlag, TypeMapper, Materializer, IdentityMD);
}
}

Expand Down Expand Up @@ -322,16 +315,19 @@ void llvm::CloneFunctionInto(Function *NewFunc, const Function *OldFunc,
DISubprogram *SPClonedWithinModule =
CollectDebugInfoForCloning(*OldFunc, Changes, DIFinder);

ModuleLevelChanges =
BuildDebugInfoMDMap(VMap.MD(), Changes, DIFinder, SPClonedWithinModule);
MetadataSetTy IdentityMD =
FindDebugInfoToIdentityMap(Changes, DIFinder, SPClonedWithinModule);

const auto RemapFlag = ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges;
// Cloning is always a Module level operation, since Metadata needs to be
// cloned.
const auto RemapFlag = RF_None;

CloneFunctionMetadataInto(*NewFunc, *OldFunc, VMap, RemapFlag, TypeMapper,
Materializer);
Materializer, &IdentityMD);

CloneFunctionBodyInto(*NewFunc, *OldFunc, VMap, RemapFlag, Returns,
NameSuffix, CodeInfo, TypeMapper, Materializer);
NameSuffix, CodeInfo, TypeMapper, Materializer,
&IdentityMD);

// Only update !llvm.dbg.cu for DifferentModule (not CloneModule). In the
// same module, the compile unit will already be listed (or not). When
Expand Down
18 changes: 14 additions & 4 deletions llvm/lib/Transforms/Utils/ValueMapper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,12 +120,14 @@ class Mapper {
SmallVector<WorklistEntry, 4> Worklist;
SmallVector<DelayedBasicBlock, 1> DelayedBBs;
SmallVector<Constant *, 16> AppendingInits;
const MetadataSetTy *IdentityMD;

public:
Mapper(ValueToValueMapTy &VM, RemapFlags Flags,
ValueMapTypeRemapper *TypeMapper, ValueMaterializer *Materializer)
ValueMapTypeRemapper *TypeMapper, ValueMaterializer *Materializer,
const MetadataSetTy *IdentityMD)
: Flags(Flags), TypeMapper(TypeMapper),
MCs(1, MappingContext(VM, Materializer)) {}
MCs(1, MappingContext(VM, Materializer)), IdentityMD(IdentityMD) {}

/// ValueMapper should explicitly call \a flush() before destruction.
~Mapper() { assert(!hasWorkToDo() && "Expected to be flushed"); }
Expand Down Expand Up @@ -899,6 +901,13 @@ std::optional<Metadata *> Mapper::mapSimpleMetadata(const Metadata *MD) {
return wrapConstantAsMetadata(*CMD, mapValue(CMD->getValue()));
}

// Map metadata from IdentityMD on first use. We need to add these nodes to
// the mapping as otherwise metadata nodes numbering gets messed up. This is
// still economical because the amount of data in IdentityMD may be a lot
// larger than what will actually get used.
if (IdentityMD && IdentityMD->contains(MD))
return getVM().MD()[MD] = TrackingMDRef(const_cast<Metadata *>(MD));

assert(isa<MDNode>(MD) && "Expected a metadata node");

return std::nullopt;
Expand Down Expand Up @@ -1198,8 +1207,9 @@ class FlushingMapper {

ValueMapper::ValueMapper(ValueToValueMapTy &VM, RemapFlags Flags,
ValueMapTypeRemapper *TypeMapper,
ValueMaterializer *Materializer)
: pImpl(new Mapper(VM, Flags, TypeMapper, Materializer)) {}
ValueMaterializer *Materializer,
const MetadataSetTy *IdentityMD)
: pImpl(new Mapper(VM, Flags, TypeMapper, Materializer, IdentityMD)) {}

ValueMapper::~ValueMapper() { delete getAsMapper(pImpl); }

Expand Down
Loading