ValueMapper: Stop memoizing MDStrings

Stop adding MDString to the Metadata section of the ValueMap in
MapMetadata.  It blows up the size of the map for no benefit, since we
can always return quickly anyway.

There is a potential follow-up that I don't think I'll push on right
away, but maybe someone else is interested:  stop checking for a
pre-mapped MDString, and move the `isa<MDString>()` checks in
Mapper::mapSimpleMetadata and MDNodeMapper::getMappedOp in front of the
`VM.getMappedMD()` calls.  While this would preclude explicitly
remapping MDStrings it would probably be a little faster.

llvm-svn: 265827

2 files changed, 21 insertions, 2 deletions

diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp
index a5981115584..b18af19a981 100644
--- a/llvm/lib/Transforms/Utils/ValueMapper.cpp
+++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp
@@ -434,7 +434,8 @@ bool MDNodeMapper::mapOperand(const Metadata *Op) {
     return false;
 
   if (Optional<Metadata *> MappedOp = M.mapSimpleMetadata(Op)) {
-    assert(M.VM.getMappedMD(Op) && "Expected result to be memoized");
+    assert((isa<MDString>(Op) || M.VM.getMappedMD(Op)) &&
+           "Expected result to be memoized");
     return *MappedOp != Op;
   }
 
@@ -448,6 +449,9 @@ Optional<Metadata *> MDNodeMapper::getMappedOp(const Metadata *Op) const {
   if (Optional<Metadata *> MappedOp = M.VM.getMappedMD(Op))
     return *MappedOp;
 
+  if (isa<MDString>(Op))
+    return const_cast<Metadata *>(Op);
+
   return None;
 }
 
@@ -649,7 +653,7 @@ Optional<Metadata *> Mapper::mapSimpleMetadata(const Metadata *MD) {
     return *NewMD;
 
   if (isa<MDString>(MD))
-    return mapToSelf(MD);
+    return const_cast<Metadata *>(MD);
 
   // This is a module-level metadata.  If nothing at the module level is
   // changing, use an identity mapping.
diff --git a/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp b/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
index ecaea460594..3bccd503c1b 100644
--- a/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
+++ b/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
@@ -140,6 +140,21 @@ TEST(ValueMapperTest, MapMetadataNullMapGlobalWithIgnoreMissingLocals) {
   EXPECT_EQ(nullptr, MapValue(F.get(), VM, Flags));
 }
 
+TEST(ValueMapperTest, MapMetadataMDString) {
+  LLVMContext C;
+  auto *S1 = MDString::get(C, "S1");
+  ValueToValueMapTy VM;
+
+  // Make sure S1 maps to itself, but isn't memoized.
+  EXPECT_EQ(S1, MapMetadata(S1, VM));
+  EXPECT_EQ(None, VM.getMappedMD(S1));
+
+  // We still expect VM.MD() to be respected.
+  auto *S2 = MDString::get(C, "S2");
+  VM.MD()[S1].reset(S2);
+  EXPECT_EQ(S2, MapMetadata(S1, VM));
+}
+
 TEST(ValueMapperTest, MapMetadataConstantAsMetadata) {
   LLVMContext C;
   FunctionType *FTy =