ValueMapper: Rename RF_MoveDistinctMDs => RF_ReuseAndMutateDistinctMD…

…s, 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.

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.

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();

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);

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;

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();

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));
 }