[llvm] r244181 - ValueMapper: Rotate distinct node remapping algorithm
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Wed Aug 5 23:10:08 PDT 2015
> 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.
> 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
>
More information about the llvm-commits
mailing list