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

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

dexonsmith added a comment.

@steven_wu , I think this bug is a dual of http://reviews.llvm.org/D20586, and the correct fix is something like:

  --- a/llvm/lib/Linker/IRMover.cpp
  +++ b/llvm/lib/Linker/IRMover.cpp
  @@ -602,20 +602,24 @@ Value *IRLinker::materialize(Value *V, bool ForIndirectSymbol) {
         return New;
     }
   
  -  // When linking a global for an indirect symbol, it will always be linked.
  -  // However we need to check if it was not already scheduled to satisfy a
  -  // reference from a regular global value initializer. We know if it has been
  -  // schedule if the "New" GlobalValue that is mapped here for the indirect
  -  // symbol is the same as the one already mapped. If there is an entry in the
  -  // ValueMap but the value is different, it means that the value already had a
  -  // definition in the destination module (linkonce for instance), but we need a
  -  // new definition for the indirect symbol ("New" will be different.
  -  if (ForIndirectSymbol && ValueMap.lookup(SGV) == New)
  +  // Decide whether the body should be linked, which is always true when
  +  // linking a global for an indirect symbol.
  +  if (!ForIndirectSymbol && !shouldLink(New, *SGV))
       return New;
   
  -  if (ForIndirectSymbol || shouldLink(New, *SGV))
  -    setError(linkGlobalValueBody(*New, *SGV));
  +  // If the global is being linked for an indirect symbol, it may have already
  +  // been scheduled to satisfy a regular symbol. Similarly, a global being linked
  +  // for a regular symbol may have already been scheduled for an indirect
  +  // symbol. Check for these cases by looking in the dual value map and
  +  // confirming the same value has been scheduled.  If there is an entry in the
  +  // ValueMap but the value is different, it means that the value already had a
  +  // definition in the destination module (linkonce for instance), but we need
  +  // a new definition for the indirect symbol ("New" will be different).
  +  if ((ForIndirectSymbol && ValueMap.lookup(SGV) == New) ||
  +      (!ForIndirectSymbol && IndirectSymbolValueMap.lookup(SGV) == New))
  +    return New;
   
  +  setError(linkGlobalValueBody(*New, *SGV));
     return New;
   }
   

I'll post an alternate patch in a moment.

Others: it'd still be useful to better understand why two value maps are needed for comdats. It'd be great to get that documented in the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99546