[PATCH] D18100: Simplify Logic in IRMover

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

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 http://reviews.llvm.org/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.


http://reviews.llvm.org/D18100