blob: 40310b4175ebcd541e412972bad345a8f8cc6468 [file] [log] [blame]
commit fa35c1f80f0ea080a7cbc581416929b0a654f25c
Author: Duncan P. N. Exon Smith <dexonsmith@apple.com>
Date: Wed Feb 10 15:23:53 2021 -0800
ValueMapper: Rename RF_MoveDistinctMDs => RF_ReuseAndMutateDistinctMDs, NFC
Rename the `RF_MoveDistinctMDs` flag passed into `MapValue` and
`MapMetadata` to `RF_ReuseAndMutateDistinctMDs` in order to more
precisely describe its effect and clarify the header documentation.
Found this while helping to investigate PR48841, which pointed out an
unsound use of the flag in `CloneModule()`. For now I've just added a
FIXME there, but I'm hopeful that the new (more precise) name will
prevent other similar errors.
diff --git a/llvm/include/llvm/Transforms/Utils/ValueMapper.h b/llvm/include/llvm/Transforms/Utils/ValueMapper.h
index ff5bfc609586..4245f51cc1e2 100644
--- a/llvm/include/llvm/Transforms/Utils/ValueMapper.h
+++ b/llvm/include/llvm/Transforms/Utils/ValueMapper.h
@@ -89,9 +89,11 @@ enum RemapFlags {
/// \a MapMetadata() always ignores this flag.
RF_IgnoreMissingLocals = 2,
- /// Instruct the remapper to move distinct metadata instead of duplicating it
- /// when there are module-level changes.
- RF_MoveDistinctMDs = 4,
+ /// Instruct the remapper to reuse and mutate distinct metadata (remapping
+ /// them in place) instead of cloning remapped copies. This flag has no
+ /// effect when when RF_NoModuleLevelChanges, since that implies an identity
+ /// mapping.
+ RF_ReuseAndMutateDistinctMDs = 4,
/// Any global values not in value map are mapped to null instead of mapping
/// to self. Illegal if RF_IgnoreMissingLocals is also set.
diff --git a/llvm/lib/IR/LLVMContextImpl.h b/llvm/lib/IR/LLVMContextImpl.h
index 05fd1814e2dc..6d5588352dfb 100644
--- a/llvm/lib/IR/LLVMContextImpl.h
+++ b/llvm/lib/IR/LLVMContextImpl.h
@@ -784,10 +784,10 @@ template <> struct MDNodeSubsetEqualImpl<DISubprogram> {
// Compare to the RHS.
// FIXME: We need to compare template parameters here to avoid incorrect
- // collisions in mapMetadata when RF_MoveDistinctMDs and a ODR-DISubprogram
- // has a non-ODR template parameter (i.e., a DICompositeType that does not
- // have an identifier). Eventually we should decouple ODR logic from
- // uniquing logic.
+ // collisions in mapMetadata when RF_ReuseAndMutateDistinctMDs and a
+ // ODR-DISubprogram has a non-ODR template parameter (i.e., a
+ // DICompositeType that does not have an identifier). Eventually we should
+ // decouple ODR logic from uniquing logic.
return IsDefinition == RHS->isDefinition() && Scope == RHS->getRawScope() &&
LinkageName == RHS->getRawLinkageName() &&
TemplateParams == RHS->getRawTemplateParams();
diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index 6a2f84bb48a0..4d7c5ef67217 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -520,8 +520,8 @@ public:
: DstM(DstM), SrcM(std::move(SrcM)), AddLazyFor(std::move(AddLazyFor)),
TypeMap(Set), GValMaterializer(*this), LValMaterializer(*this),
SharedMDs(SharedMDs), IsPerformingImport(IsPerformingImport),
- Mapper(ValueMap, RF_MoveDistinctMDs | RF_IgnoreMissingLocals, &TypeMap,
- &GValMaterializer),
+ Mapper(ValueMap, RF_ReuseAndMutateDistinctMDs | RF_IgnoreMissingLocals,
+ &TypeMap, &GValMaterializer),
IndirectSymbolMCID(Mapper.registerAlternateMappingContext(
IndirectSymbolValueMap, &LValMaterializer)) {
ValueMap.getMDMap() = std::move(SharedMDs);
diff --git a/llvm/lib/Transforms/Utils/CloneModule.cpp b/llvm/lib/Transforms/Utils/CloneModule.cpp
index 6de679bc9640..487cd4eae957 100644
--- a/llvm/lib/Transforms/Utils/CloneModule.cpp
+++ b/llvm/lib/Transforms/Utils/CloneModule.cpp
@@ -120,9 +120,13 @@ std::unique_ptr<Module> llvm::CloneModule(
SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
G.getAllMetadata(MDs);
+
+ // FIXME: Stop using RF_ReuseAndMutateDistinctMDs here, since it's unsound
+ // to mutate metadata that is still referenced by the source module unless
+ // the source is about to be discarded (see IRMover for a valid use).
for (auto MD : MDs)
- GV->addMetadata(MD.first,
- *MapMetadata(MD.second, VMap, RF_MoveDistinctMDs));
+ GV->addMetadata(MD.first, *MapMetadata(MD.second, VMap,
+ RF_ReuseAndMutateDistinctMDs));
if (G.isDeclaration())
continue;
diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp
index 930e0b7ee01a..9557fa9becf0 100644
--- a/llvm/lib/Transforms/Utils/ValueMapper.cpp
+++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp
@@ -547,7 +547,7 @@ MDNode *MDNodeMapper::mapDistinctNode(const MDNode &N) {
assert(N.isDistinct() && "Expected a distinct node");
assert(!M.getVM().getMappedMD(&N) && "Expected an unmapped node");
DistinctWorklist.push_back(
- cast<MDNode>((M.Flags & RF_MoveDistinctMDs)
+ cast<MDNode>((M.Flags & RF_ReuseAndMutateDistinctMDs)
? M.mapToSelf(&N)
: M.mapToMetadata(&N, cloneOrBuildODR(N))));
return DistinctWorklist.back();
diff --git a/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp b/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
index a586ac7bb20a..abfa67143463 100644
--- a/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
+++ b/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
@@ -124,7 +124,7 @@ TEST(ValueMapperTest, mapMDNodeDistinct) {
{
// The node should be moved.
ValueToValueMapTy VM;
- EXPECT_EQ(D, ValueMapper(VM, RF_MoveDistinctMDs).mapMDNode(*D));
+ EXPECT_EQ(D, ValueMapper(VM, RF_ReuseAndMutateDistinctMDs).mapMDNode(*D));
}
}
@@ -139,7 +139,7 @@ TEST(ValueMapperTest, mapMDNodeDistinctOperands) {
VM.MD()[Old].reset(New);
// Make sure operands are updated.
- EXPECT_EQ(D, ValueMapper(VM, RF_MoveDistinctMDs).mapMDNode(*D));
+ EXPECT_EQ(D, ValueMapper(VM, RF_ReuseAndMutateDistinctMDs).mapMDNode(*D));
EXPECT_EQ(New, D->getOperand(0));
}