[llvm] r277625 - IR: Drop uniquing when an MDNode Value operand is deleted
Hans Wennborg via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 3 13:02:32 PDT 2016
Thanks! Merged in r277639.
On Wed, Aug 3, 2016 at 11:38 AM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
> 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