Simplify Logic in IRMover

[mehdi_amini]

Summary

I'm not sure about correctness here. The test-suite is passing though.

[tobiasvk]

lib/Linker/IRMover.cpp

1067

Why can you drop "if (ShouldLink)"? That seems unrelated to merging the two maps.

[mehdi_amini]

lib/Linker/IRMover.cpp

1067

I'm not sure, can I ask the opposite question: "why not?"? ;) (this code is not documented so I have no clue...)

My impression is that if there is already a value in the map, then we should just use it. Why would linkGlobalValueProto() create a new one?
Maybe it is an invariant of the method that we can't have !ShouldLink and the value being in the map. In this case the test if(ShouldLink) is just here to save querying the map. That may be my best bet, so I'll add the check back, but an assert in the else branch.

[tobiasvk]

lib/Linker/IRMover.cpp

1067

To be honest, I have no clue either ;)

Adding an assert sounds like a good option - at least it won't silently break if this is in fact needed.

[mehdi_amini]

Enforce invariant with an assertion

[tejohnson]

I applied this patch and successfully built SPEC cpu2006 with both full and thin LTO. However, I'd like to wait until Rafael takes a look since he added this as part of his patch to split the linker in 2.

Tracing through the code this morning, I where this change will make a difference in how the mapping works. Because we use a separate map when linking an aliasee definition, and also use a different materializer LValMaterialize of type LocalValueMaterializer, the ForAlias flag gets set to true. If you look at IRLinker::linkGlobalValueProto you can see where this will make a difference. Since we are using a different value map, it seems like we should be trying to materialize GVs that may even have been mapped already (in the ValueMap but not the AliasValueMap). Then when we call back into linkGlobalValueProto when mapping the Aliasee body for these encountered GVs, ForAlias will be true. Here are some relevant snippets from linkGlobalValueProto, with my own comments added:

// Teresa: this will essentially force the subsequent code to create a NewGV for this GV
if (!ShouldLink && ForAlias)
  DGV = nullptr;

<snipped the mapping code>

// Teresa: This will cause the NewGV to be placed in a COMDAT if necessary
if (ShouldLink || ForAlias) {
  if (const Comdat *SC = SGV->getComdat()) {
    if (auto *GO = dyn_cast<GlobalObject>(NewGV)) {
      Comdat *DC = DstM.getOrInsertComdat(SC->getName());
      DC->setSelectionKind(SC->getSelectionKind());
      GO->setComdat(DC);
    }
  }
}

// Teresa: Here is a key difference - GV is mapped (possibly after already being mapped by due to a non-aliasee reference), and the new copy is internal.
if (!ShouldLink && ForAlias)
  NewGV->setLinkage(GlobalValue::InternalLinkage);

I vaguely recall Rafael discussing linking aliasees as internal - aha, see the discussion relating to aliasees and comdats in D14623. I believe this change was to implement his suggested fix. Also see test/Linker/alias.ll which was modified to check this as part of the linker splitting change. However, that is still passing with this patch applied...

As an aside, if this change is ok and accepted by Rafael, the LocalValueMaterializer and instances of ForAlias should also be removed as they would be dead.

[• rafael]

This breaks tools/gold/X86/comdat.ll

lib/Linker/IRMover.cpp

class IRLinker {
   GlobalValueMaterializer GValMaterializer;
   LocalValueMaterializer LValMaterializer;
 
   /// Mapping of values from what they used to be in Src, to what they are now
   /// in DstM.  ValueToValueMapTy is a ValueMap, which involves some overhead
   /// due to the use of Value handles which the Linker doesn't actually need,
   /// but this allows us to reuse the ValueMapper code.
   ValueToValueMapTy ValueMap;
-  ValueToValueMapTy AliasValueMap;
 
   DenseSet<GlobalValue *> ValuesToLink;
   std::vector<GlobalValue *> Worklist;
 
   void maybeAdd(GlobalValue *GV) {
     if (ValuesToLink.insert(GV).second)
       Worklist.push_back(GV);
   }
bool IRLinker::shouldLink(GlobalValue *DGV, GlobalValue &SGV) {
   return LazilyAdded;
 }
 
 Constant *IRLinker::linkGlobalValueProto(GlobalValue *SGV, bool ForAlias) {
   GlobalValue *DGV = getLinkedToGlobal(SGV);
 
   bool ShouldLink = shouldLink(DGV, *SGV);
 
-  // just missing from map
+  /// First verify if the value hasn't been alreay linked, which should not be
+  /// possible without ShouldLink
   if (ShouldLink) {
     auto I = ValueMap.find(SGV);
     if (I != ValueMap.end())
       return cast<Constant>(I->second);
-
-    I = AliasValueMap.find(SGV);
-    if (I != AliasValueMap.end())
-      return cast<Constant>(I->second);
+  } else {
+    assert(ValueMap.find(SGV) == ValueMap.end() &&
+           "'!ShouldLink' GV shouldn't be in the ValueMap, broken invariant");
   }
 
   if (!ShouldLink && ForAlias)
     DGV = nullptr;
 
   // Handle the ultra special appending linkage case first.
   assert(!DGV || SGV->hasAppendingLinkage() == DGV->hasAppendingLinkage());
   if (SGV->hasAppendingLinkage())
     return linkAppendingVarProto(cast_or_null<GlobalVariable>(DGV),
bool IRLinker::linkFunctionBody(Function &Dst, Function &Src) {
   for (Argument &Arg : Src.args())
     ValueMap.erase(&Arg);
 
   return false;
 }
 
 void IRLinker::linkAliasBody(GlobalAlias &Dst, GlobalAlias &Src) {
   Constant *Aliasee = Src.getAliasee();
-  Constant *Val = MapValue(Aliasee, AliasValueMap, ValueMapperFlags, &TypeMap,
+  Constant *Val = MapValue(Aliasee, ValueMap, ValueMapperFlags, &TypeMap,
                            &LValMaterializer);
   Dst.setAliasee(Val);
 }
 
 bool IRLinker::linkGlobalValueBody(GlobalValue &Dst, GlobalValue &Src) {
   if (auto *F = dyn_cast<Function>(&Src))
     return linkFunctionBody(cast<Function>(Dst), *F);
   if (auto *GVar = dyn_cast<GlobalVariable>(&Src)) {
bool IRLinker::run() {
   computeTypeMapping();
 
   std::reverse(Worklist.begin(), Worklist.end());
   while (!Worklist.empty()) {
     GlobalValue *GV = Worklist.back();
     Worklist.pop_back();
 
     // Already mapped.
-    if (ValueMap.find(GV) != ValueMap.end() ||
-        AliasValueMap.find(GV) != AliasValueMap.end())
+    if (ValueMap.find(GV) != ValueMap.end())
       continue;
 
     assert(!GV->isDeclaration());
     MapValue(GV, ValueMap, ValueMapperFlags, &TypeMap, &GValMaterializer);
     if (HasError)
       return true;
   }