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

[Rafael Espíndola via llvm-commits]

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

Sure, PR is fine. Even longer term I want to completely remove the
appending linkage, but that will take some time.


The "FIXME: Why is this upgrade done during linking?" is duplicated btw.

you can probably use std::find for getMappingContextID. I wonder if it
wouldn't be cleaner to have the caller do something like

unsigned NormalID = Mapper.registerContext(...);
unsigned AliasID = Mapped.registerContext(...);

and pass the context to scheduleMap*.


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

Cool. BTW, do you intend to also port CloneModule and CloneFunction to
use the ValueMapper class? With that you should be able to remove the
top level function. This can be another patch.

In any case, LGTM with nits and a few suggestions.

Cheers,
Rafael