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