[llvm] r267099 - ValueMapper/Enumerator: Clean up code in post-order traversals, NFC
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Thu Apr 21 20:46:00 PDT 2016
> On Apr 21, 2016, at 7:33 PM, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> Author: dexonsmith
> Date: Thu Apr 21 21:33:06 2016
> New Revision: 267099
>
> URL: http://llvm.org/viewvc/llvm-project?rev=267099&view=rev
> Log:
> ValueMapper/Enumerator: Clean up code in post-order traversals, NFC
>
> Re-layer the functions in the new (i.e., newly correct) post-order
> traversals in ValueEnumerator (r266947) and ValueMapper (r266949).
> Instead of adding a node to the worklist in a helper function and
> returning a flag to say what happened, return the node itself. This
> makes the code way cleaner: the worklist is local to the main function,
> there is no flag for an early loop exit (since we can cleanly bury the
> loop), and it's perfectly clear when pointers into the worklist might be
> invalidated.
>
> I'm fixing both algorithms in the same commit to avoid repeating the
> commit message; if you take the time to understand one the other should
> be easy. The diff itself isn't entirely obvious since the traversals
> have some noise (i.e., things to do), but here's the high-level change:
>
> auto helper = [&WL](T *Op) { auto helper = [](T **&I, T **E) {
> => while (I != E) {
> if (shouldVisit(Op)) { T *Op = *I++;
> WL.push(Op, Op->begin()); if (shouldVisit(Op)) {
> return true; return Op;
> } }
> return false; return nullptr;
> }; };
> =>
> WL.push(S, S->begin()); WL.push(S, S->begin());
> while (!empty()) { while (!empty()) {
> auto *N = WL.top().N; auto *N = WL.top().N;
> auto *&I = WL.top().I; auto *&I = WL.top().I;
> bool DidChange = false;
> while (I != N->end())
> if (helper(*I++)) { => if (T *Op = helper(I, N->end()) {
> DidChange = true; WL.push(Op, Op->begin());
> break; continue;
> } }
> if (DidChange)
> continue;
>
> POT.push(WL.pop()); => POT.push(WL.pop());
> } }
Nice commit message :)
>
> Thanks to Mehdi for helping me find a better way to layer this.
>
> Modified:
> llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.cpp
> llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.h
> llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp
>
> Modified: llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.cpp?rev=267099&r1=267098&r2=267099&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.cpp (original)
> +++ llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.cpp Thu Apr 21 21:33:06 2016
> @@ -569,36 +569,40 @@ void ValueEnumerator::dropFunctionFromMe
> void ValueEnumerator::EnumerateMetadata(unsigned F, const Metadata *MD) {
> // Start by enumerating MD, and then work through its transitive operands in
> // post-order. This requires a depth-first search.
> - SmallVector<std::pair<const MDNode *, const MDOperand *>, 32> Worklist;
> - enumerateMetadataImpl(F, MD, Worklist);
> + SmallVector<std::pair<const MDNode *, MDNode::op_iterator>, 32> Worklist;
> + if (const MDNode *N = enumerateMetadataImpl(F, MD))
> + Worklist.push_back(std::make_pair(N, N->op_begin()));
> +
> while (!Worklist.empty()) {
> const MDNode *N = Worklist.back().first;
> - const MDOperand *&Op = Worklist.back().second; // Be careful of lifetime...
> + MDNode::op_iterator &I = Worklist.back().second;
>
> // Enumerate operands until the worklist changes. We need to traverse new
> // nodes before visiting the rest of N's operands.
> - bool DidWorklistChange = false;
> - for (const MDOperand *E = N->op_end(); Op != E;)
> - if (enumerateMetadataImpl(F, *Op++, Worklist)) {
> - DidWorklistChange = true;
> - break;
> - }
> - if (DidWorklistChange)
> + if (const MDNode *Op = enumerateMetadataOperands(F, I, N->op_end())) {
> + Worklist.push_back(std::make_pair(Op, Op->op_begin()));
Now that you have nicely refactored it, you almost have a std algorithm at hand.
I'd kill enumerateMetadataOperands (there is no other use) and avoid the side effect increment on I (which is not obvious from the call site).
Something like this:
// Enumerate operands until the worklist changes. We need to traverse new
// nodes before visiting the rest of N's operands.
I = llvm::find_if(
MDNode::op_range(I, N->op_end()),
[&](const MDOperand &Op) { return enumerateMetadataImpl(F, Op); });
if (I != N->op_end()) {
// We find a new operand to process, queue it and continue
const MDNode *MD = dyn_cast<MDNode>(*I);
Worklist.push_back(std::make_pair(MD, MD->op_begin()));
continue;
}
--
Mehdi
> continue;
> + }
>
> // All the operands have been visited. Now assign an ID.
> Worklist.pop_back();
> MDs.push_back(N);
> MetadataMap[N].ID = MDs.size();
> - continue;
> }
> }
>
> -bool ValueEnumerator::enumerateMetadataImpl(
> - unsigned F, const Metadata *MD,
> - SmallVectorImpl<std::pair<const MDNode *, const MDOperand *>> &Worklist) {
> +const MDNode *
> +ValueEnumerator::enumerateMetadataOperands(unsigned F, MDNode::op_iterator &I,
> + MDNode::op_iterator E) {
> + while (I != E)
> + if (const MDNode *N = enumerateMetadataImpl(F, *I++)) // Always increment I.
> + return N;
> + return nullptr;
> +}
> +
> +const MDNode *ValueEnumerator::enumerateMetadataImpl(unsigned F, const Metadata *MD) {
> if (!MD)
> - return false;
> + return nullptr;
>
> assert(
> (isa<MDNode>(MD) || isa<MDString>(MD) || isa<ConstantAsMetadata>(MD)) &&
> @@ -610,14 +614,12 @@ bool ValueEnumerator::enumerateMetadataI
> // Already mapped. If F doesn't match the function tag, drop it.
> if (Entry.hasDifferentFunction(F))
> dropFunctionFromMetadata(*Insertion.first);
> - return false;
> + return nullptr;
> }
>
> - // MDNodes are handled separately to avoid recursion.
> - if (auto *N = dyn_cast<MDNode>(MD)) {
> - Worklist.push_back(std::make_pair(N, N->op_begin()));
> - return true; // Changed the worklist.
> - }
> + // Don't assign IDs to metadata nodes.
> + if (auto *N = dyn_cast<MDNode>(MD))
> + return N;
>
> // Save the metadata.
> MDs.push_back(MD);
> @@ -627,7 +629,7 @@ bool ValueEnumerator::enumerateMetadataI
> if (auto *C = dyn_cast<ConstantAsMetadata>(MD))
> EnumerateValue(C->getValue());
>
> - return false;
> + return nullptr;
> }
>
> /// EnumerateFunctionLocalMetadataa - Incorporate function-local metadata
>
> Modified: llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.h?rev=267099&r1=267098&r2=267099&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.h (original)
> +++ llvm/trunk/lib/Bitcode/Writer/ValueEnumerator.h Thu Apr 21 21:33:06 2016
> @@ -246,9 +246,24 @@ private:
> /// function.
> void incorporateFunctionMetadata(const Function &F);
>
> - bool enumerateMetadataImpl(
> - unsigned F, const Metadata *MD,
> - SmallVectorImpl<std::pair<const MDNode *, const MDOperand *>> &Worklist);
> + /// Enumerate operands with the given function tag.
> + ///
> + /// Enumerate the Metadata operands between \c I and \c E, returning the
> + /// first newly-enumerated MDNode without assigning it an ID.
> + ///
> + /// \post If a node was found, \c I points just past the node.
> + /// \post If no node was found, \c I is equal to \c E.
> + const MDNode *enumerateMetadataOperands(unsigned F, const MDOperand *&I,
> + const MDOperand *E);
> +
> + /// Enumerate a single instance of metadata with the given function tag.
> + ///
> + /// If \c MD has already been enumerated, check that \c F matches its
> + /// function tag. If not, call \a dropFunctionFromMetadata().
> + ///
> + /// Otherwise, mark \c MD as visited. Assign it an ID, or just return it if
> + /// it's an \a MDNode.
> + const MDNode *enumerateMetadataImpl(unsigned F, const Metadata *MD);
>
> unsigned getMetadataFunctionID(const Function *F) const;
> void EnumerateMetadata(const Function *F, const Metadata *MD);
>
> Modified: llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp?rev=267099&r1=267098&r2=267099&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp (original)
> +++ llvm/trunk/lib/Transforms/Utils/ValueMapper.cpp Thu Apr 21 21:33:06 2016
> @@ -228,18 +228,6 @@ class MDNodeMapper {
> Metadata &getFwdReference(MDNode &Op);
> };
>
> - /// An entry in the worklist for the post-order traversal.
> - struct POTWorklistEntry {
> - MDNode *N; ///< Current node.
> - const MDOperand *Op; ///< Current operand of \c N.
> -
> - /// Keep a flag of whether operands have changed in the worklist to avoid
> - /// hitting the map in \a UniquedGraph.
> - bool HasChanged = false;
> -
> - POTWorklistEntry(MDNode &N) : N(&N), Op(N.op_begin()) {}
> - };
> -
> /// Worklist of distinct nodes whose operands need to be remapped.
> SmallVector<MDNode *, 16> DistinctWorklist;
>
> @@ -330,11 +318,15 @@ private:
> /// to change because of operands outside the graph.
> bool createPOT(UniquedGraph &G, const MDNode &FirstN);
>
> - /// Visit an operand of a node in the POT.
> + /// Visit the operands of a uniqued node in the POT.
> ///
> - /// Return \c true iff a new node was pushed onto \c Worklist.
> - bool visitOperand(UniquedGraph &G, Metadata *Op,
> - SmallVectorImpl<POTWorklistEntry> &Worklist);
> + /// Visit the operands in the range from \c I to \c E, returning the first
> + /// uniqued node we find that isn't yet in \c G. \c I is always advanced to
> + /// where to continue the loop through the operands.
> + ///
> + /// This sets \c HasChanged if any of the visited operands change.
> + MDNode *visitOperands(UniquedGraph &G, MDNode::op_iterator &I,
> + MDNode::op_iterator E, bool &HasChanged);
>
> /// Map all the nodes in the given uniqued graph.
> ///
> @@ -609,6 +601,20 @@ void MDNodeMapper::remapOperands(MDNode
> }
> }
>
> +namespace {
> +/// An entry in the worklist for the post-order traversal.
> +struct POTWorklistEntry {
> + MDNode *N; ///< Current node.
> + MDNode::op_iterator Op; ///< Current operand of \c N.
> +
> + /// Keep a flag of whether operands have changed in the worklist to avoid
> + /// hitting the map in \a UniquedGraph.
> + bool HasChanged = false;
> +
> + POTWorklistEntry(MDNode &N) : N(&N), Op(N.op_begin()) {}
> +};
> +} // end namespace
> +
> bool MDNodeMapper::createPOT(UniquedGraph &G, const MDNode &FirstN) {
> assert(G.Info.empty() && "Expected a fresh traversal");
> assert(FirstN.isUniqued() && "Expected uniqued node in POT");
> @@ -619,49 +625,46 @@ bool MDNodeMapper::createPOT(UniquedGrap
> Worklist.push_back(POTWorklistEntry(const_cast<MDNode &>(FirstN)));
> (void)G.Info[&FirstN];
> while (!Worklist.empty()) {
> - MDNode &N = *Worklist.back().N;
> - const MDOperand *&Op = Worklist.back().Op; // Careful of lifetime...
> - assert(N.isUniqued() && "Expected only uniqued nodes in POT");
> -
> - // Pick up the traversal from Op and continue. Since this is a DFS, pause
> - // as soon as a new node is pushed onto the worklist.
> - bool DidWorklistSizeChange = false;
> - for (const MDOperand *E = N.op_end(); Op != E;) {
> - assert(*Op != &N && "Uniqued nodes cannot have self-references");
> - if (visitOperand(G, *Op++, Worklist)) {
> - DidWorklistSizeChange = true;
> - break;
> - }
> - }
> - if (DidWorklistSizeChange)
> + // Start or continue the traversal through the this node's operands.
> + auto &WE = Worklist.back();
> + if (MDNode *N = visitOperands(G, WE.Op, WE.N->op_end(), WE.HasChanged)) {
> + // Push a new node to traverse first.
> + Worklist.push_back(POTWorklistEntry(*N));
> continue;
> + }
>
> - // All operands of N have been visited. Push N into the POT.
> - auto &D = G.Info[&N];
> - AnyChanges |= D.HasChanged = Worklist.pop_back_val().HasChanged;
> + // Push the node onto the POT.
> + assert(WE.N->isUniqued() && "Expected only uniqued nodes");
> + assert(WE.Op == WE.N->op_end() && "Expected to visit all operands");
> + auto &D = G.Info[WE.N];
> + AnyChanges |= D.HasChanged = WE.HasChanged;
> D.ID = G.POT.size();
> - G.POT.push_back(&N);
> + G.POT.push_back(WE.N);
> +
> + // Pop the node off the worklist.
> + Worklist.pop_back();
> }
> return AnyChanges;
> }
>
> -bool MDNodeMapper::visitOperand(UniquedGraph &G, Metadata *Op,
> - SmallVectorImpl<POTWorklistEntry> &Worklist) {
> - // Try to map Op, and check it for changes.
> - if (Optional<Metadata *> MappedOp = tryToMapOperand(Op)) {
> - Worklist.back().HasChanged |= Op != *MappedOp;
> - return false;
> - }
> -
> - // Push Op onto the Worklist unless it's already in G.
> - MDNode &OpN = *cast<MDNode>(Op);
> - assert(OpN.isUniqued() &&
> - "Only uniqued operands cannot be mapped immediately");
> - if (!G.Info.insert(std::make_pair(&OpN, Data())).second)
> - return false;
> +MDNode *MDNodeMapper::visitOperands(UniquedGraph &G, MDNode::op_iterator &I,
> + MDNode::op_iterator E, bool &HasChanged) {
> + while (I != E) {
> + Metadata *Op = *I++; // Increment even on early return.
> + if (Optional<Metadata *> MappedOp = tryToMapOperand(Op)) {
> + // Check if the operand changes.
> + HasChanged |= Op != *MappedOp;
> + continue;
> + }
>
> - Worklist.push_back(POTWorklistEntry(OpN));
> - return true;
> + // A uniqued metadata node.
> + MDNode &OpN = *cast<MDNode>(Op);
> + assert(OpN.isUniqued() &&
> + "Only uniqued operands cannot be mapped immediately");
> + if (G.Info.insert(std::make_pair(&OpN, Data())).second)
> + return &OpN; // This is a new one. Return it.
> + }
> + return nullptr;
> }
>
> void MDNodeMapper::UniquedGraph::propagateChanges() {
>
>
> _______________________________________________
> 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