[llvm] r277625 - IR: Drop uniquing when an MDNode Value operand is deleted
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 3 11:38:42 PDT 2016
Hi Hans,
Mehdi nominated this for 3.9 during review, and I second.
Thanks,
Duncan
> On 2016-Aug-03, at 11:19, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> Author: dexonsmith
> Date: Wed Aug 3 13:19:43 2016
> New Revision: 277625
>
> URL: http://llvm.org/viewvc/llvm-project?rev=277625&view=rev
> Log:
> IR: Drop uniquing when an MDNode Value operand is deleted
>
> This is a fix for PR28697.
>
> An MDNode can indirectly refer to a GlobalValue, through a
> ConstantAsMetadata. When the GlobalValue is deleted, the MDNode operand
> is reset to `nullptr`. If the node is uniqued, this can lead to a
> hard-to-detect cache invalidation in a Metadata map that's shared across
> an LLVMContext.
>
> Consider:
>
> 1. A map from Metadata* to `T` called RemappedMDs.
> 2. A node that references a global variable, `!{i1* @GV}`.
> 3. Insert `!{i1* @GV} -> SomeT` in the map.
> 4. Delete `@GV`, leaving behind `!{null} -> SomeT`.
>
> Looking up the generic and uninteresting `!{null}` gives you `SomeT`,
> which is likely related to `@GV`. Worse, `SomeT`'s lifetime may be tied
> to the deleted `@GV`.
>
> This occurs in practice in the shared ValueMap used since r266579 in the
> IRMover. Other code that handles more than one Module (with different
> lifetimes) in the same LLVMContext could hit it too.
>
> The fix here is a partial revert of r225223: in the rare case that an
> MDNode operand is a ConstantAsMetadata (i.e., wrapping a node from the
> Value hierarchy), drop uniquing if it gets replaced with `nullptr`.
> This changes step #4 above to leave behind `distinct !{null} -> SomeT`,
> which can't be confused with the generic `!{null}`.
>
> In theory, this can cause some churn in the LLVMContext's MDNode
> uniquing map when Values are being deleted. However:
>
> - The number of GlobalValues referenced from uniqued MDNodes is
> expected to be quite small. E.g., the debug info metadata schema
> only references GlobalValues from distinct nodes.
>
> - Other Constants have the lifetime of the LLVMContext, whose teardown
> is careful to drop references before deleting the constants.
>
> As a result, I don't expect a compile time regression from this change.
>
> Added:
> llvm/trunk/test/Linker/Inputs/metadata-with-global-value-operand.ll
> llvm/trunk/test/Linker/metadata-with-global-value-operand.ll
> Modified:
> llvm/trunk/lib/IR/Metadata.cpp
> llvm/trunk/test/Transforms/GlobalOpt/metadata.ll
> llvm/trunk/unittests/IR/MetadataTest.cpp
>
> Modified: llvm/trunk/lib/IR/Metadata.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/Metadata.cpp?rev=277625&r1=277624&r2=277625&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/Metadata.cpp (original)
> +++ llvm/trunk/lib/IR/Metadata.cpp Wed Aug 3 13:19:43 2016
> @@ -675,8 +675,8 @@ void MDNode::handleChangedOperand(void *
> Metadata *Old = getOperand(Op);
> setOperand(Op, New);
>
> - // Drop uniquing for self-reference cycles.
> - if (New == this) {
> + // Drop uniquing for self-reference cycles and deleted constants.
> + if (New == this || (!New && Old && isa<ConstantAsMetadata>(Old))) {
> if (!isResolved())
> resolve();
> storeDistinctInContext();
>
> Added: llvm/trunk/test/Linker/Inputs/metadata-with-global-value-operand.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/Inputs/metadata-with-global-value-operand.ll?rev=277625&view=auto
> ==============================================================================
> --- llvm/trunk/test/Linker/Inputs/metadata-with-global-value-operand.ll (added)
> +++ llvm/trunk/test/Linker/Inputs/metadata-with-global-value-operand.ll Wed Aug 3 13:19:43 2016
> @@ -0,0 +1,3 @@
> +!named.null = !{!0}
> +
> +!0 = !{null}
>
> Added: llvm/trunk/test/Linker/metadata-with-global-value-operand.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Linker/metadata-with-global-value-operand.ll?rev=277625&view=auto
> ==============================================================================
> --- llvm/trunk/test/Linker/metadata-with-global-value-operand.ll (added)
> +++ llvm/trunk/test/Linker/metadata-with-global-value-operand.ll Wed Aug 3 13:19:43 2016
> @@ -0,0 +1,14 @@
> +; RUN: llvm-link -S -o - %s %S/Inputs/metadata-with-global-value-operand.ll | FileCheck %s
> +; This test confirms that the !{null} from the second module doesn't get mapped
> +; onto the abandoned !{i1* @var} node from this module.
> +
> +; CHECK: @var = global
> + at var = global i1 false
> +
> +; CHECK: !named.vars = !{!0}
> +; CHECK: !named.null = !{!1}
> +!named.vars = !{!0}
> +
> +; CHECK: !0 = !{i1* @var}
> +; CHECK: !1 = !{null}
> +!0 = !{i1* @var}
>
> Modified: llvm/trunk/test/Transforms/GlobalOpt/metadata.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GlobalOpt/metadata.ll?rev=277625&r1=277624&r2=277625&view=diff
> ==============================================================================
> --- llvm/trunk/test/Transforms/GlobalOpt/metadata.ll (original)
> +++ llvm/trunk/test/Transforms/GlobalOpt/metadata.ll Wed Aug 3 13:19:43 2016
> @@ -28,5 +28,5 @@ declare void @llvm.foo(metadata, metadat
> ; CHECK: !named = !{![[NULL:[0-9]+]]}
>
> !0 = !{i8*** @G}
> -; CHECK-DAG: ![[NULL]] = !{null}
> +; CHECK-DAG: ![[NULL]] = distinct !{null}
> ; CHECK-DAG: ![[EMPTY]] = !{}
>
> Modified: llvm/trunk/unittests/IR/MetadataTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/MetadataTest.cpp?rev=277625&r1=277624&r2=277625&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/IR/MetadataTest.cpp (original)
> +++ llvm/trunk/unittests/IR/MetadataTest.cpp Wed Aug 3 13:19:43 2016
> @@ -449,6 +449,40 @@ TEST_F(MDNodeTest, DistinctOnUniquingCol
> EXPECT_FALSE(Wrapped1->isDistinct());
> }
>
> +TEST_F(MDNodeTest, UniquedOnDeletedOperand) {
> + // temp !{}
> + TempMDTuple T = MDTuple::getTemporary(Context, None);
> +
> + // !{temp !{}}
> + Metadata *Ops[] = {T.get()};
> + MDTuple *N = MDTuple::get(Context, Ops);
> +
> + // !{temp !{}} => !{null}
> + T.reset();
> + ASSERT_TRUE(N->isUniqued());
> + Metadata *NullOps[] = {nullptr};
> + ASSERT_EQ(N, MDTuple::get(Context, NullOps));
> +}
> +
> +TEST_F(MDNodeTest, DistinctOnDeletedValueOperand) {
> + // i1* @GV
> + Type *Ty = Type::getInt1PtrTy(Context);
> + std::unique_ptr<GlobalVariable> GV(
> + new GlobalVariable(Ty, false, GlobalValue::ExternalLinkage));
> + ConstantAsMetadata *Op = ConstantAsMetadata::get(GV.get());
> +
> + // !{i1* @GV}
> + Metadata *Ops[] = {Op};
> + MDTuple *N = MDTuple::get(Context, Ops);
> +
> + // !{i1* @GV} => !{null}
> + GV.reset();
> + ASSERT_TRUE(N->isDistinct());
> + ASSERT_EQ(nullptr, N->getOperand(0));
> + Metadata *NullOps[] = {nullptr};
> + ASSERT_NE(N, MDTuple::get(Context, NullOps));
> +}
> +
> TEST_F(MDNodeTest, getDistinct) {
> // !{}
> MDNode *Empty = MDNode::get(Context, None);
> @@ -669,7 +703,7 @@ TEST_F(MDNodeTest, replaceWithUniquedRes
> EXPECT_TRUE(N->isResolved());
> }
>
> -TEST_F(MDNodeTest, replaceWithUniquedChangingOperand) {
> +TEST_F(MDNodeTest, replaceWithUniquedDeletedOperand) {
> // i1* @GV
> Type *Ty = Type::getInt1PtrTy(Context);
> std::unique_ptr<GlobalVariable> GV(
> @@ -686,8 +720,33 @@ TEST_F(MDNodeTest, replaceWithUniquedCha
>
> // !{i1* @GV} => !{null}
> GV.reset();
> - ASSERT_TRUE(N->isUniqued());
> + ASSERT_TRUE(N->isDistinct());
> + ASSERT_EQ(nullptr, N->getOperand(0));
> Metadata *NullOps[] = {nullptr};
> + ASSERT_NE(N, MDTuple::get(Context, NullOps));
> +}
> +
> +TEST_F(MDNodeTest, replaceWithUniquedChangedOperand) {
> + // i1* @GV
> + Type *Ty = Type::getInt1PtrTy(Context);
> + std::unique_ptr<GlobalVariable> GV(
> + new GlobalVariable(Ty, false, GlobalValue::ExternalLinkage));
> + ConstantAsMetadata *Op = ConstantAsMetadata::get(GV.get());
> +
> + // temp !{i1* @GV}
> + Metadata *Ops[] = {Op};
> + MDTuple *N = MDTuple::getTemporary(Context, Ops).release();
> +
> + // temp !{i1* @GV} => !{i1* @GV}
> + ASSERT_EQ(N, MDNode::replaceWithUniqued(TempMDTuple(N)));
> + ASSERT_TRUE(N->isUniqued());
> +
> + // !{i1* @GV} => !{i1* @GV2}
> + std::unique_ptr<GlobalVariable> GV2(
> + new GlobalVariable(Ty, false, GlobalValue::ExternalLinkage));
> + GV->replaceAllUsesWith(GV2.get());
> + ASSERT_TRUE(N->isUniqued());
> + Metadata *NullOps[] = {ConstantAsMetadata::get(GV2.get())};
> ASSERT_EQ(N, MDTuple::get(Context, NullOps));
> }
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
More information about the llvm-commits
mailing list