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