[PATCH] ValueMapper: Rewrite Mapper::mapMetadata without recursion

[Duncan P. N. Exon Smith via llvm-commits]

> 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?)

> 
> +  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.

> 
> 
> +   /// \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