From a77d073305acde5afa0b61daf3fe1a823ed8ad81 Mon Sep 17 00:00:00 2001
From: "Duncan P. N. Exon Smith" <dexonsmith@apple.com>
Date: Sat, 16 Apr 2016 03:39:44 +0000
Subject: [PATCH] ValueMapper: Stop memoizing ConstantAsMetadata

Stop memoizing ConstantAsMetadata in ValueMapper::mapMetadata.  Now we
have to recompute it, but these metadata aren't particularly common, and
it restricts the lifetime of the Metadata map unnecessarily.

(The motivation is that I have a patch which uses a single Metadata map
for the lifetime of IRMover.  Mehdi profiled r266446 with the patch
applied and we saw a pretty big speedup in lib/Linker.)

llvm-svn: 266513
---
 llvm/lib/Transforms/Utils/ValueMapper.cpp          | 45 +++++++++++++++++-----
 .../unittests/Transforms/Utils/ValueMapperTest.cpp |  9 +++--
 2 files changed, 41 insertions(+), 13 deletions(-)

diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp
index db984ee..d12ae92 100644
--- a/llvm/lib/Transforms/Utils/ValueMapper.cpp
+++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp
@@ -505,14 +505,26 @@ bool MDNodeMapper::mapOperand(const Metadata *Op) {
     return false;
 
   if (Optional<Metadata *> MappedOp = M.mapSimpleMetadata(Op)) {
-    assert((isa<MDString>(Op) || M.getVM().getMappedMD(Op)) &&
-           "Expected result to be memoized");
+    if (auto *CMD = dyn_cast<ConstantAsMetadata>(Op))
+      assert((!*MappedOp || M.getVM().count(CMD->getValue()) ||
+              M.getVM().getMappedMD(Op)) &&
+             "Expected Value to be memoized");
+    else
+      assert((isa<MDString>(Op) || M.getVM().getMappedMD(Op)) &&
+             "Expected result to be memoized");
     return *MappedOp != Op;
   }
 
   return push(*cast<MDNode>(Op)).HasChangedAddress;
 }
 
+static ConstantAsMetadata *wrapConstantAsMetadata(const ConstantAsMetadata &CMD,
+                                                  Value *MappedV) {
+  if (CMD.getValue() == MappedV)
+    return const_cast<ConstantAsMetadata *>(&CMD);
+  return MappedV ? ConstantAsMetadata::getConstant(MappedV) : nullptr;
+}
+
 Optional<Metadata *> MDNodeMapper::getMappedOp(const Metadata *Op) const {
   if (!Op)
     return nullptr;
@@ -523,6 +535,9 @@ Optional<Metadata *> MDNodeMapper::getMappedOp(const Metadata *Op) const {
   if (isa<MDString>(Op))
     return const_cast<Metadata *>(Op);
 
+  if (auto *CMD = dyn_cast<ConstantAsMetadata>(Op))
+    return wrapConstantAsMetadata(*CMD, M.getVM().lookup(CMD->getValue()));
+
   return None;
 }
 
@@ -720,6 +735,19 @@ Metadata *MDNodeMapper::map(const MDNode &FirstN) {
   return *getMappedOp(&FirstN);
 }
 
+namespace {
+
+struct MapMetadataDisabler {
+  ValueToValueMapTy &VM;
+
+  MapMetadataDisabler(ValueToValueMapTy &VM) : VM(VM) {
+    VM.disableMapMetadata();
+  }
+  ~MapMetadataDisabler() { VM.enableMapMetadata(); }
+};
+
+} // end namespace
+
 Optional<Metadata *> Mapper::mapSimpleMetadata(const Metadata *MD) {
   // If the value already exists in the map, use it.
   if (Optional<Metadata *> NewMD = getVM().getMappedMD(MD))
@@ -735,14 +763,13 @@ Optional<Metadata *> Mapper::mapSimpleMetadata(const Metadata *MD) {
 
   if (auto *CMD = dyn_cast<ConstantAsMetadata>(MD)) {
     // Disallow recursion into metadata mapping through mapValue.
-    getVM().disableMapMetadata();
-    Value *MappedV = mapValue(CMD->getValue());
-    getVM().enableMapMetadata();
-
-    if (CMD->getValue() == MappedV)
-      return mapToSelf(MD);
+    MapMetadataDisabler MMD(getVM());
 
-    return mapToMetadata(MD, MappedV ? ValueAsMetadata::get(MappedV) : nullptr);
+    // Don't memoize ConstantAsMetadata.  Instead of lasting until the
+    // LLVMContext is destroyed, they can be deleted when the GlobalValue they
+    // reference is destructed.  These aren't super common, so the extra
+    // indirection isn't that expensive.
+    return wrapConstantAsMetadata(*CMD, mapValue(CMD->getValue()));
   }
 
   assert(isa<MDNode>(MD) && "Expected a metadata node");
diff --git a/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp b/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
index 2c6d45a..34b62bb 100644
--- a/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
+++ b/llvm/unittests/Transforms/Utils/ValueMapperTest.cpp
@@ -238,13 +238,14 @@ TEST(ValueMapperTest, mapMetadataConstantAsMetadata) {
 
   auto *CAM = ConstantAsMetadata::get(F.get());
   {
+    // ConstantAsMetadata shouldn't be memoized.
     ValueToValueMapTy VM;
     EXPECT_EQ(CAM, ValueMapper(VM).mapMetadata(*CAM));
-    EXPECT_TRUE(VM.MD().count(CAM));
-    VM.MD().erase(CAM);
+    EXPECT_FALSE(VM.MD().count(CAM));
     EXPECT_EQ(CAM, ValueMapper(VM, RF_IgnoreMissingLocals).mapMetadata(*CAM));
-    EXPECT_TRUE(VM.MD().count(CAM));
+    EXPECT_FALSE(VM.MD().count(CAM));
 
+    // But it should respect a mapping that gets seeded.
     auto *N = MDTuple::get(C, None);
     VM.MD()[CAM].reset(N);
     EXPECT_EQ(N, ValueMapper(VM).mapMetadata(*CAM));
@@ -256,7 +257,7 @@ TEST(ValueMapperTest, mapMetadataConstantAsMetadata) {
   ValueToValueMapTy VM;
   VM[F.get()] = F2.get();
   auto *F2MD = ValueMapper(VM).mapMetadata(*CAM);
-  EXPECT_TRUE(VM.MD().count(CAM));
+  EXPECT_FALSE(VM.MD().count(CAM));
   EXPECT_TRUE(F2MD);
   EXPECT_EQ(F2.get(), cast<ConstantAsMetadata>(F2MD)->getValue());
 }
-- 
2.7.4