[PATCH] ValueMapper: Rewrite Mapper::mapMetadata without recursion
Mehdi Amini via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 5 14:04:06 PDT 2016
> On Apr 5, 2016, at 1:33 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
>>
>> On 2016-Apr-05, at 11:39, Mehdi Amini <mehdi.amini at apple.com> wrote:
>>
>>>
>>> On Apr 3, 2016, at 10:51 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>
>>> This commit completely rewrites Mapper::mapMetadata (the implementation
>>> of llvm::MapMetadata) using an iterative algorithm. The guts of the new
>>> algorithm are in MDNodeMapper::map, the entry function in a new class.
>>>
>>> Previously, Mapper::mapMetadata performed a recursive exploration of the
>>> graph with eager "just in case there's a reason" malloc traffic.
>>>
>>> The new algorithm has these benefits:
>>>
>>> - New nodes and temporaries are not created eagerly.
>>> - Uniquing cycles are not duplicated (see new unit test).
>>> - No recursion.
>>>
>>> Given a node to map, it does this:
>>>
>>> 1. Use a worklist to perform a post-order traversal of the transitively
>>> referenced unmapped nodes.
>>>
>>> 2. Track which nodes will change operands, and which will have new
>>> addresses in the mapped scheme. Propagate the changes through the
>>> POT until fixed point, to pick up uniquing cycles that need to
>>> change.
>>>
>>> 3. Map all the distinct nodes without touching their operands. If
>>> RF_MoveDistinctMetadata, they get mapped to themselves; otherwise,
>>> they get mapped to clones.
>>>
>>> 4. Map the uniqued nodes (bottom-up), lazily creating temporaries for
>>> forward references as needed.
>>>
>>> 5. Remap the operands of the distinct nodes.
>>>
>>> MapMetadata barely shows up in the profiles of llvm-link I've looked at
>>> with the new algorithm.
>>>
>>> It would be nice to break the strange remaining recursion on the Value
>>> side: MapValue => materializeInitFor => RemapInstruction => MapValue. I
>>> think we could do this by somehow leaking (at least an opaque version
>>> of) the "Mapper" context into the API, perhaps by sending a callback
>>> function to materializeInitFor.
>>>
>>>
>>> <0001-ValueMapper-Rewrite-Mapper-mapMetadata-without-re-v1.patch>
>>
>>
>>
>> The CPU profiling does not show a great improvement (~10-15%), because to many map query I think.
>> The memory profile is better with close to 50% less persistent memory, 33% less transient memory, but also a lot less transient memory allocations.
>> On my example there were 2.5M allocations before and now there are only 266k allocations! A lot let stressful for the memory subsystem :)
>
> FYI, Mehdi and I looked at this at his desk, and it dropped from 2.5M down
> to 26k, about 100x fewer.
>
>> On the code itself now, MDNodeMapper::map() is fairly clear but for the interaction between createPOT() and propagateChangedOperands().
>> The former says it is doing "a traversal of the unmapped subgraph, checking for changed operands”, which make it surprising that you need propagateChangedOperands() afterward.
>
> I cleaned up the comment a little. It does check for changed
> operands, which maybe is a bit confusing.
>
>> Below just two nits:
>
> Committed r265456. I thought I remembered an LGTM here, but looking
> again I don't see it; sorry for jumping the gun :(. (I can revert?)
No, it is fine, I didn’t have anything else to add.
>
>>
>> + assert(!(M.Flags & RF_NoModuleLevelChanges) &&
>> + "MDNodeMapper::map assumes module-level changes");
>>
>>
>> I think the string is wrong, it should be: "MDNodeMapper::map assumes NO module-level changes”.
>
> The string is correct. There's a double negation in the assertion.
Oh right, I mis-read the string (but “map assumes no no-module-level changes” would be really weird).
—
Mehdi
>
>>
>>
>> + /// \post Data::HasChangedAddress == RF_MoveDistinctMDs if N.isDistinct().
>>
>> But the code is:
>>
>> + D.HasChangedAddress = !(M.Flags & RF_MoveDistinctMDs) && N.isDistinct();
>>
>> Which does not seem to match the comment, I think the comment is wrong and should be:
>>
>> + /// \post Data::HasChangedAddress == !RF_MoveDistinctMDs if N.isDistinct().
>
> Nice catch. Fixed!
>
>>
>>
>>
>>
>>
>> —
>> Mehdi
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160405/93bb058e/attachment.html>
More information about the llvm-commits
mailing list