[PATCH] ValueMapper: Eliminate cross-file co-recursion, NFC

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

> On 2016-Apr-12, at 19:40, Rafael Espíndola <rafael.espindola at gmail.com> wrote:
> 
> On 12 April 2016 at 19:28, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> Eliminate co-recursion of Mapper::mapValue through
>> ValueMaterializer::materializeInitFor, through a major redesign of the
>> ValueMapper.cpp interface.
>> 
>>  - Expose a ValueMapper class that controls the entry points to the
>>    mapping algorithms.
>>  - Change IRLinker to use ValueMapper directly, rather than
>>    llvm::RemapInstruction, llvm::MapValue, etc.
>>  - Use (e.g.) ValueMapper::scheduleMapGlobalInit to add mapping work to
>>    a worklist in ValueMapper instead of recursing.
>> 
>> There were two fairly major complications.
>> 
>> Firstly, IRLinker::linkAppendingVarProto incorporates an on-the-fly IR
>> ugprade that I had to split apart.  Frankly I think this upgrade should
>> be done in the bitcode reader (and we should only accept the "new"
>> form), but for now I've just made it work and added a FIXME.
> 
> The issue is not bitcode, is support for old clients and the C api.

Ah, the C API.  I vaguely remembered pushing to do the upgrade when this
went in and was confused to see the logic.  It all makes sense now.

> Now that we have proper deprecation procedures we should probably do
> it. In this case we are not deprecating a symbol (after all it is just
> creating a GlobalVariable), we are deprecating what the expected
> content should be.

I can file a PR.  It's not really urgent.

>> Secondly, IRLinker has special logic to correctly implement aliases with
>> comdats, and uses two ValueToValueMapTy instances and two
>> ValueMaterializers.  This wasn't hard to support by making ValueMapper
>> support arbitrary MappingContexts when scheduling work.
> 
> Thanks for taking care of this odd case.
> 
>> While out of scope for this commit, it should be straightforward to
>> remove recursion from Mapper::mapValue once this is in place.
> 
> Out of curiosity, is this making the algorithm more efficient,
> avoiding overflowing the stack or is just a cleanup?

It's mainly a cleanup, so that it's easier to reason about and to see
results in a CPU profile, etc. (I find the nesting really strange).
However I would like to break the recursion in mapValue, too, so that
we don't have to worry about it overflowing the stack in extreme cases.