[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