[PATCH] D99546: [llvm][ValueMap] Share the value map between MappingContext

Tue Apr 27 19:10:41 PDT 2021

tejohnson added a subscriber: respindola.
tejohnson added a comment.

In D99546#2721266 <https://reviews.llvm.org/D99546#2721266>, @dexonsmith wrote:

> In D99546#2657116 <https://reviews.llvm.org/D99546#2657116>, @steven_wu wrote:
>
>> @pcc @tejohnson I tried to fix the assertion failure shown in the test case (reduced from actual code generated from swift compiler) by simplifying the ValueMapper but I couldn't quite get the comdat tests to pass. I think comdat is relying on some of the indirect symbol that doesn't appear in the ValueMap for the normal context but I couldn't quite figure out how that works. Any suggestion?
>
> @pcc or @tejohnson (or maybe @MaskRay from the lld side), any chance you could help explain why the testcases llvm/test/Linker/comdat16.ll and llvm/test/LTO/Resolution/X86/comdat16.ll rely on a separate value map? Or do you have a suggestion for someone else that could help?

I'm afraid I'm not going to be much help, at least not without more investigation. The 2 comdat tests mentioned above (I think the second one is supposed to be llvm/test/LTO/Resolution/X86/comdat.ll?) were added after the separate value map for aliases already existed, and weren't related to any value mapper changes afaict. I just did some archeology and found that the separate map exists as far back as when the IRMover code was introduced by @respindola in 2015, being split from older IR linking code, which also seems to have had a separate value map for aliases (didn't look any further back though, the code was structured quite differently).

In what way do they break with this change?

> Restating the context, maybe with a bit more detail:
>
> - The IR in the testcase attached to the patch is reduced IR from the Swift compiler.
> - It triggers an assertion in the IRLinker, which was added ~when Metadata was split out of the Value class hierarchy:
>
>   void Mapper::scheduleMapGlobalInitializer(GlobalVariable &GV, Constant &Init,
>                                             unsigned MCID) {
>     assert(AlreadyScheduled.insert(&GV).second && "Should not reschedule");
>
> - The `MCID` is a mapping context ID. There are two mapping contexts in the IRMover: one for most values, and separate one for aliases / indirect symbols. `scheduleMapGlobalInitializer()` is adding `GV` to a worklist, used to break an otherwise-deep recursion between IRMover and ValueMapper. The assertion fails because the symbol is being scheduled in *both* contexts.
> - Steven's proposed fix (removing `IndirectSymbolValueMap` but keeping the materializer) fixes the testcase above. However, it's incomplete, as it breaks the two comdat testcases.
>
> I don't remember much about this code, although I'm trying to page it back in; IIRC, when I broke the recursion I tried removing the second value map; that broke these same testcases, and I created this "mapping context" ID in order to make progress, but I'm not sure I fully investigated what it was for.
>
> I'm hoping someone that knows more about comdats can help us piece together the motivation here: why do comdats need a separate value map for aliases? (Maybe there's even a simpler/cleaner way to model the requirement.)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99546/new/

https://reviews.llvm.org/D99546