[llvm] fa35c1f - ValueMapper: Rename RF_MoveDistinctMDs => RF_ReuseAndMutateDistinctMDs, NFC

[Duncan P. N. Exon Smith via llvm-commits]

Author: Duncan P. N. Exon Smith
Date: 2021-02-10T16:53:21-08:00
New Revision: fa35c1f80f0ea080a7cbc581416929b0a654f25c

URL: https://github.com/llvm/llvm-project/commit/fa35c1f80f0ea080a7cbc581416929b0a654f25c
DIFF: https://github.com/llvm/llvm-project/commit/fa35c1f80f0ea080a7cbc581416929b0a654f25c.diff

LOG: 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.

Added: 
    

Modified: 
    llvm/include/llvm/Transforms/Utils/ValueMapper.h
    llvm/lib/IR/LLVMContextImpl.h
    llvm/lib/Linker/IRMover.cpp
    llvm/lib/Transforms/Utils/CloneModule.cpp
    llvm/lib/Transforms/Utils/ValueMapper.cpp
    llvm/unittests/Transforms/Utils/ValueMapperTest.cpp

Removed: 
    


################################################################################
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 @@ class IRLinker {
       : 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));
 }