[llvm] r266513 - ValueMapper: Stop memoizing ConstantAsMetadata
Craig Topper via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 15 22:08:25 PDT 2016
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.
> + 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160415/2a59c5cd/attachment.html>
More information about the llvm-commits
mailing list