Page MenuHomePhabricator

Simplify Logic in IRMover
Needs ReviewPublic

Authored by mehdi_amini on Mar 11 2016, 2:22 PM.

Details

Summary

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

Diff Detail

Event Timeline

mehdi_amini retitled this revision from to Simplify Logic in IRMover.
mehdi_amini updated this object.
mehdi_amini added reviewers: tejohnson, rafael.
mehdi_amini added a subscriber: llvm-commits.
tobiasvk added inline comments.
lib/Linker/IRMover.cpp
1067

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

mehdi_amini added inline comments.Mar 11 2016, 2:53 PM
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 added inline comments.Mar 11 2016, 3:08 PM
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.

Enforce invariant with an assertion

tejohnson edited edge metadata.Mar 15 2016, 8:25 AM

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 edited edge metadata.Mar 16 2016, 6:47 PM
rafael added a subscriber: rafael.

This breaks tools/gold/X86/comdat.ll

espindola edited reviewers, added: espindola; removed: rafael.Mar 15 2018, 10:53 AM