<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 5, 2015 at 11:10 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">> On 2015-Aug-05, at 17:25, Sean Silva <<a href="mailto:chisophugis@gmail.com">chisophugis@gmail.com</a>> wrote:<br>
><br>
> Is there actually a use case for uniqueable metadata that relies on the ability to form cycles?<br>
><br>
<br>
</span>Debug info, unfortunately, still uses these.<br>
<br>
The biggest sources of cycles (that I know about):<br>
<br>
- DISubprogram (variables) <-> DILocalVariable (scope chain).<br>
- DICompositeType (members) <-> DIDerivedType (base type) when type<br>
uniquing doesn't apply.<br>
<br>
There may be others, but they're not on my radar.<br>
<br>
Anyway, I'm working on removing them. This commit is a step toward<br>
that: make 'distinct' nodes simpler and cheaper.<br></blockquote><div><br></div><div>Cool. Thanks for all the hard work!</div><div><br></div><div>-- Sean Silva</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
> On Wed, Aug 5, 2015 at 4:52 PM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> Author: dexonsmith<br>
> Date: Wed Aug 5 18:52:42 2015<br>
> New Revision: 244181<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=244181&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=244181&view=rev</a><br>
> Log:<br>
> ValueMapper: Rotate distinct node remapping algorithm<br>
><br>
> Rotate the algorithm for remapping distinct nodes in order to simplify<br>
> how uniquing cycles get resolved. This removes some of the recursion,<br>
> and, most importantly, exposes all uniquing cycles at the top-level.<br>
> Besides being a little more efficient -- temporary MDNodes won't live as<br>
> long -- the clearer logic should help protect against bugs like those<br>
> fixed in r243961 and r243976.<br>
><br>
> What are uniquing cycles? Why do they present challenges when remapping<br>
> metadata?<br>
><br>
> !0 = !{!1}<br>
> !1 = !{!0}<br>
><br>
> !0 and !1 form a simple uniquing cycle. When remapping from one<br>
> metadata graph to another, every uniquing cycle gets "duplicated"<br>
> through a dance:<br>
><br>
> !0-temp = !{!1?} ; map(!0): clone !0, VM[!0] = !0-temp<br>
> !1-temp = !{!0?} ; ..map(!1): clone !1, VM[!1] = !1-temp<br>
> !1-temp = !{!0-temp} ; ..map(!1): remap !1's operands<br>
> !2 = !{!0-temp} ; ..map(!1): uniquify: !1-temp => !2<br>
> !0-temp = !{!2} ; map(!0): remap !0's operands<br>
> !3 = !{!2} ; map(!0): uniquify: !0-temp => !3<br>
><br>
> ; Result<br>
> !2 = !{!3}<br>
> !3 = !{!2}<br>
><br>
> (In the two "uniquify" steps above, the operands of !X-temp are compared<br>
> to the operands of !X. If they're the same, then !X-temp gets RAUW'ed<br>
> to !X; if they're different, then !X-temp is promoted to a new unique<br>
> node. The latter case always hits in for uniquing cycles, so we<br>
> duplicate all the nodes involved.)<br>
><br>
> Why is this a problem? Uniquable Metadata nodes that have temporary<br>
> node as transitive operands keep RAUW support until the temporary nodes<br>
> get finalized. With non-cycles, this happens automatically: when a<br>
> uniquable node's count of unresolved operands drops to zero, it<br>
> immediately sheds its own RAUW support (possibly triggering the same in<br>
> any node that references it). However, uniquing cycles create a<br>
> reference cycle, and uniqued nodes that transitively reference a<br>
> uniquing cycle are "stuck" in an unresolved state until someone calls<br>
> `MDNode::resolveCycles()` on a node in the unresolved subgraph.<br>
><br>
> Distinct nodes should help here (and mostly do): since they aren't<br>
> uniqued anywhere, they are guaranteed not to be RAUW'ed. They<br>
> effectively form a barrier between uniqued nodes, breaking some uniquing<br>
> cycles, and shielding uniqued nodes from uniquing cycles.<br>
><br>
> Unfortunately, with this barrier in place, the unresolved subgraph(s)<br>
> can be disjoint from the top-level node. The mapping algorithm needs to<br>
> find at least one representative from each disjoint subgraph. But which<br>
> nodes are *stuck*, and which will get resolved automatically? And which<br>
> nodes are in the unresolved subgraph? The old logic was conservative.<br>
><br>
> This commit rotates the logic for distinct nodes, so that we have access<br>
> to unresolved nodes at the top-level call to `llvm::MapMetadata()`.<br>
> Each time we return to the top-level, we know that all temporaries have<br>
> been RAUW'ed away. Here, it's safe (and necessary) to call<br>
> `resolveCycles()` immediately on unresolved operands.<br>
><br>
> This should also perform better than the old algorithm. The recursion<br>
> stack is shorter, temporary nodes don't live as long, and there are<br>
> fewer tracking references to unresolved nodes. As the debug info graph<br>
> introduces more 'distinct' nodes, remapping should incrementally get<br>
> cheaper and cheaper.<br>
><br>
> Aside from possible performance improvements (and reduced cruft in the<br>
> `LLVMContext`), there should be no functionality change here.<br>
><br>
> Modified:<br>
> llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp<br>
><br>
> Modified: llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp?rev=244181&r1=244180&r2=244181&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp?rev=244181&r1=244180&r2=244181&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp (original)<br>
> +++ llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp Wed Aug 5 18:52:42 2015<br>
> @@ -156,20 +156,20 @@ static Metadata *mapToSelf(ValueToValueM<br>
> }<br>
><br>
> static Metadata *MapMetadataImpl(const Metadata *MD,<br>
> - SmallVectorImpl<TrackingMDNodeRef> &Cycles,<br>
> + SmallVectorImpl<MDNode *> &DistinctWorklist,<br>
> ValueToValueMapTy &VM, RemapFlags Flags,<br>
> ValueMapTypeRemapper *TypeMapper,<br>
> ValueMaterializer *Materializer);<br>
><br>
> static Metadata *mapMetadataOp(Metadata *Op,<br>
> - SmallVectorImpl<TrackingMDNodeRef> &Cycles,<br>
> + SmallVectorImpl<MDNode *> &DistinctWorklist,<br>
> ValueToValueMapTy &VM, RemapFlags Flags,<br>
> ValueMapTypeRemapper *TypeMapper,<br>
> ValueMaterializer *Materializer) {<br>
> if (!Op)<br>
> return nullptr;<br>
> - if (Metadata *MappedOp =<br>
> - MapMetadataImpl(Op, Cycles, VM, Flags, TypeMapper, Materializer))<br>
> + if (Metadata *MappedOp = MapMetadataImpl(Op, DistinctWorklist, VM, Flags,<br>
> + TypeMapper, Materializer))<br>
> return MappedOp;<br>
> // Use identity map if MappedOp is null and we can ignore missing entries.<br>
> if (Flags & RF_IgnoreMissingEntries)<br>
> @@ -185,7 +185,7 @@ static Metadata *mapMetadataOp(Metadata<br>
><br>
> /// Remap the operands of an MDNode.<br>
> static bool remapOperands(MDNode &Node,<br>
> - SmallVectorImpl<TrackingMDNodeRef> &Cycles,<br>
> + SmallVectorImpl<MDNode *> &DistinctWorklist,<br>
> ValueToValueMapTy &VM, RemapFlags Flags,<br>
> ValueMapTypeRemapper *TypeMapper,<br>
> ValueMaterializer *Materializer) {<br>
> @@ -194,8 +194,8 @@ static bool remapOperands(MDNode &Node,<br>
> bool AnyChanged = false;<br>
> for (unsigned I = 0, E = Node.getNumOperands(); I != E; ++I) {<br>
> Metadata *Old = Node.getOperand(I);<br>
> - Metadata *New =<br>
> - mapMetadataOp(Old, Cycles, VM, Flags, TypeMapper, Materializer);<br>
> + Metadata *New = mapMetadataOp(Old, DistinctWorklist, VM, Flags, TypeMapper,<br>
> + Materializer);<br>
> if (Old != New) {<br>
> AnyChanged = true;<br>
> Node.replaceOperandWith(I, New);<br>
> @@ -212,7 +212,7 @@ static bool remapOperands(MDNode &Node,<br>
> /// place; effectively, they're moved from one graph to another. Otherwise,<br>
> /// they're cloned/duplicated, and the new copy's operands are remapped.<br>
> static Metadata *mapDistinctNode(const MDNode *Node,<br>
> - SmallVectorImpl<TrackingMDNodeRef> &Cycles,<br>
> + SmallVectorImpl<MDNode *> &DistinctWorklist,<br>
> ValueToValueMapTy &VM, RemapFlags Flags,<br>
> ValueMapTypeRemapper *TypeMapper,<br>
> ValueMaterializer *Materializer) {<br>
> @@ -224,23 +224,16 @@ static Metadata *mapDistinctNode(const M<br>
> else<br>
> NewMD = MDNode::replaceWithDistinct(Node->clone());<br>
><br>
> - // Remap the operands. If any change, track those that could be involved in<br>
> - // uniquing cycles.<br>
> - mapToMetadata(VM, Node, NewMD);<br>
> - if (remapOperands(*NewMD, Cycles, VM, Flags, TypeMapper, Materializer))<br>
> - for (Metadata *Op : NewMD->operands())<br>
> - if (auto *Node = dyn_cast_or_null<MDNode>(Op))<br>
> - if (!Node->isResolved())<br>
> - Cycles.emplace_back(Node);<br>
> -<br>
> - return NewMD;<br>
> + // Remap operands later.<br>
> + DistinctWorklist.push_back(NewMD);<br>
> + return mapToMetadata(VM, Node, NewMD);<br>
> }<br>
><br>
> /// \brief Map a uniqued MDNode.<br>
> ///<br>
> /// Uniqued nodes may not need to be recreated (they may map to themselves).<br>
> static Metadata *mapUniquedNode(const MDNode *Node,<br>
> - SmallVectorImpl<TrackingMDNodeRef> &Cycles,<br>
> + SmallVectorImpl<MDNode *> &DistinctWorklist,<br>
> ValueToValueMapTy &VM, RemapFlags Flags,<br>
> ValueMapTypeRemapper *TypeMapper,<br>
> ValueMaterializer *Materializer) {<br>
> @@ -251,7 +244,8 @@ static Metadata *mapUniquedNode(const MD<br>
> // returning.<br>
> auto ClonedMD = Node->clone();<br>
> mapToMetadata(VM, Node, ClonedMD.get());<br>
> - if (!remapOperands(*ClonedMD, Cycles, VM, Flags, TypeMapper, Materializer)) {<br>
> + if (!remapOperands(*ClonedMD, DistinctWorklist, VM, Flags, TypeMapper,<br>
> + Materializer)) {<br>
> // No operands changed, so use the original.<br>
> ClonedMD->replaceAllUsesWith(const_cast<MDNode *>(Node));<br>
> return const_cast<MDNode *>(Node);<br>
> @@ -262,7 +256,7 @@ static Metadata *mapUniquedNode(const MD<br>
> }<br>
><br>
> static Metadata *MapMetadataImpl(const Metadata *MD,<br>
> - SmallVectorImpl<TrackingMDNodeRef> &Cycles,<br>
> + SmallVectorImpl<MDNode *> &DistinctWorklist,<br>
> ValueToValueMapTy &VM, RemapFlags Flags,<br>
> ValueMapTypeRemapper *TypeMapper,<br>
> ValueMaterializer *Materializer) {<br>
> @@ -307,32 +301,44 @@ static Metadata *MapMetadataImpl(const M<br>
> assert(Node->isResolved() && "Unexpected unresolved node");<br>
><br>
> if (Node->isDistinct())<br>
> - return mapDistinctNode(Node, Cycles, VM, Flags, TypeMapper, Materializer);<br>
> + return mapDistinctNode(Node, DistinctWorklist, VM, Flags, TypeMapper,<br>
> + Materializer);<br>
><br>
> - return mapUniquedNode(Node, Cycles, VM, Flags, TypeMapper, Materializer);<br>
> + return mapUniquedNode(Node, DistinctWorklist, VM, Flags, TypeMapper,<br>
> + Materializer);<br>
> }<br>
><br>
> Metadata *llvm::MapMetadata(const Metadata *MD, ValueToValueMapTy &VM,<br>
> RemapFlags Flags, ValueMapTypeRemapper *TypeMapper,<br>
> ValueMaterializer *Materializer) {<br>
> - SmallVector<TrackingMDNodeRef, 8> Cycles;<br>
> - Metadata *NewMD =<br>
> - MapMetadataImpl(MD, Cycles, VM, Flags, TypeMapper, Materializer);<br>
> -<br>
> - if ((Flags & RF_NoModuleLevelChanges) ||<br>
> - (MD == NewMD && !(Flags & RF_MoveDistinctMDs))) {<br>
> - assert(Cycles.empty() && "Unresolved cycles without remapping anything?");<br>
> + SmallVector<MDNode *, 8> DistinctWorklist;<br>
> + Metadata *NewMD = MapMetadataImpl(MD, DistinctWorklist, VM, Flags, TypeMapper,<br>
> + Materializer);<br>
> +<br>
> + // When there are no module-level changes, it's possible that the metadata<br>
> + // graph has temporaries. Skip the logic to resolve cycles, since it's<br>
> + // unnecessary (and invalid) in that case.<br>
> + if (Flags & RF_NoModuleLevelChanges)<br>
> return NewMD;<br>
> - }<br>
><br>
> + // If the top-level metadata was a uniqued MDNode, it could be involved in a<br>
> + // uniquing cycle.<br>
> if (auto *N = dyn_cast<MDNode>(NewMD))<br>
> if (!N->isResolved())<br>
> N->resolveCycles();<br>
><br>
> - // Resolve cycles underneath MD.<br>
> - for (MDNode *N : Cycles)<br>
> - if (!N->isResolved())<br>
> - N->resolveCycles();<br>
> + // Remap the operands of distinct MDNodes.<br>
> + while (!DistinctWorklist.empty()) {<br>
> + auto *N = DistinctWorklist.pop_back_val();<br>
> +<br>
> + // If an operand changes, then it may be involved in a uniquing cycle.<br>
> + if (remapOperands(*N, DistinctWorklist, VM, Flags, TypeMapper,<br>
> + Materializer))<br>
> + for (Metadata *MD : N->operands())<br>
> + if (auto *Op = dyn_cast_or_null<MDNode>(MD))<br>
> + if (!Op->isResolved())<br>
> + Op->resolveCycles();<br>
> + }<br>
><br>
> return NewMD;<br>
> }<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
><br>
<br>
</div></div></blockquote></div><br></div></div>