[llvm] r267099 - ValueMapper/Enumerator: Clean up code in post-order traversals, NFC
Duncan P. N. Exon Smith via llvm-commits
llvm-commits at lists.llvm.org
Fri Apr 22 08:52:13 PDT 2016
>> 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()));
^ Note this.
>> +
>> 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;
> }
I tried this just now (using std::find_if, which fits even better than
llvm::find_if). It looked great, until I realized I had to duplicate
the boiler-plate check and push_back insertion up above to initialize
the worklist. I could add a helper for the push, but I don't think
it's strictly better than what's already there. I think I prefer it
as is.
More information about the llvm-commits
mailing list