[llvm] r266513 - ValueMapper: Stop memoizing ConstantAsMetadata

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

> On 2016-Apr-15, at 22:08, Craig Topper <craig.topper at gmail.com> wrote:
> 
> 
> 
> On Fri, Apr 15, 2016 at 8:39 PM, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> Author: dexonsmith
> Date: Fri Apr 15 22:39:44 2016
> New Revision: 266513
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=266513&view=rev
> Log:
> 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.)
> 
> Modified:
>     llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp
>     llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp
> 
> Modified: llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp?rev=266513&r1=266512&r2=266513&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp Fri Apr 15 22:39:44 2016
> @@ -505,14 +505,26 @@ bool MDNodeMapper::mapOperand(const Meta
>      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))
> 
> CMD is unused in Release builds triggering a -Wunused-variable warning.

Sorry for the breakage (and slow response).  Simon Atanasyan fixed
this in r266529.

>  
> +      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::getMa
>    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
>    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::mapSimpleMe
> 
>    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");
> 
> Modified: llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp?rev=266513&r1=266512&r2=266513&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp (original)
> +++ llvm/trunk/unittests/Transforms/Utils/ValueMapperTest.cpp Fri Apr 15 22:39:44 2016
> @@ -238,13 +238,14 @@ TEST(ValueMapperTest, mapMetadataConstan
> 
>    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, mapMetadataConstan
>    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());
>  }
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> 
> 
> 
> -- 
> ~Craig