<div dir="ltr">Is there actually a use case for uniqueable metadata that relies on the ability to form cycles?<div><br><div>-- Sean Silva</div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 5, 2015 at 4:52 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">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>
</blockquote></div><br></div>