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

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

dexonsmith added a subscriber: MaskRay.
dexonsmith added a comment.

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?

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