[PATCH] D29367: LTO: Link non-prevailing weak_odr, linkonce_odr or available_externally globals into the combined module with available_externally linkage.

Wed Feb 1 17:36:31 PST 2017

pcc marked an inline comment as done.
pcc added inline comments.

================
Comment at: llvm/lib/LTO/LTO.cpp:484
+        if (!CombinedGV || CombinedGV->isDeclaration()) {
+          Keep.push_back(GV);
+          GV->setLinkage(GlobalValue::AvailableExternallyLinkage);
----------------
tejohnson wrote:
> pcc wrote:
> > tejohnson wrote:
> > > I don't think adding to the Keep is technically necessary because of http://llvm-cs.pcc.me.uk/lib/Linker/IRMover.cpp#873 (we always link in available_externally copies in the IRLinker). That's presumably why inglorion's D29366 worked which didn't do this, although I had to look it up because I was initially confused as to why it worked without adding to the Keep! IMO it is clearer to have the explicit add to Keep here and not rely on the IRLinker behavior - but in either case perhaps leave a comment (particularly if you end up removing it). If you end up removing it, then incoming available_externally should be removed.
> > I was also surprised to see that code in IRMover that always links available_externally, but I suspect that it may be the right place for it (linker-independent "optimisations" based on linkage).
> > 
> > If we accept that to be the correct place for it, that raises the question of whether this code itself should live in IRMover. I'm not sure, so I left a FIXME.
> > 
> > I went ahead and changed this code to set linkage to available_externally unconditionally for non-prevailing. That revealed a bug of sorts in IRMover in that it would repeatedly link available_externally definitions from source modules if they were available, instead of being satisfied with the first one. I also fixed that in this patch. A separate test failure revealed that we need to inhibit this for globals referenced by aliases, which I've also done.
> > A separate test failure revealed that we need to inhibit this for globals referenced by aliases, which I've also done.
> 
> Ah - I should have noticed that, we have to do the same thing for ThinLTO (thinLTOResolveWeakForLinkerGUID).
> 
> > If we accept that to be the correct place for it, that raises the question of whether this code itself should live in IRMover. I'm not sure, so I left a FIXME.
> 
> Do we have the linker resolution available there? Presumably we'd have to wire it in, but it seems like this might be better suited to staying in LTO.cpp which is designed to handle linker resolution info.
> Do we have the linker resolution available there? Presumably we'd have to wire it in, but it seems like this might be better suited to staying in LTO.cpp which is designed to handle linker resolution info.

Yes, we have the set `ValuesToLink`. I was imagining that IRLinker could link `*_odr` globals and change their linkage to `available_externally` if they were not members of `ValuesToLink`, similar to how it is already handling `available_externally` globals.

================
Comment at: llvm/lib/LTO/LTO.cpp:484
+        if (!CombinedGV || CombinedGV->isDeclaration()) {
+          Keep.push_back(GV);
+          GV->setLinkage(GlobalValue::AvailableExternallyLinkage);
----------------
mehdi_amini wrote:
> pcc wrote:
> > tejohnson wrote:
> > > pcc wrote:
> > > > tejohnson wrote:
> > > > > I don't think adding to the Keep is technically necessary because of http://llvm-cs.pcc.me.uk/lib/Linker/IRMover.cpp#873 (we always link in available_externally copies in the IRLinker). That's presumably why inglorion's D29366 worked which didn't do this, although I had to look it up because I was initially confused as to why it worked without adding to the Keep! IMO it is clearer to have the explicit add to Keep here and not rely on the IRLinker behavior - but in either case perhaps leave a comment (particularly if you end up removing it). If you end up removing it, then incoming available_externally should be removed.
> > > > I was also surprised to see that code in IRMover that always links available_externally, but I suspect that it may be the right place for it (linker-independent "optimisations" based on linkage).
> > > > 
> > > > If we accept that to be the correct place for it, that raises the question of whether this code itself should live in IRMover. I'm not sure, so I left a FIXME.
> > > > 
> > > > I went ahead and changed this code to set linkage to available_externally unconditionally for non-prevailing. That revealed a bug of sorts in IRMover in that it would repeatedly link available_externally definitions from source modules if they were available, instead of being satisfied with the first one. I also fixed that in this patch. A separate test failure revealed that we need to inhibit this for globals referenced by aliases, which I've also done.
> > > > A separate test failure revealed that we need to inhibit this for globals referenced by aliases, which I've also done.
> > > 
> > > Ah - I should have noticed that, we have to do the same thing for ThinLTO (thinLTOResolveWeakForLinkerGUID).
> > > 
> > > > If we accept that to be the correct place for it, that raises the question of whether this code itself should live in IRMover. I'm not sure, so I left a FIXME.
> > > 
> > > Do we have the linker resolution available there? Presumably we'd have to wire it in, but it seems like this might be better suited to staying in LTO.cpp which is designed to handle linker resolution info.
> > > Do we have the linker resolution available there? Presumably we'd have to wire it in, but it seems like this might be better suited to staying in LTO.cpp which is designed to handle linker resolution info.
> > 
> > Yes, we have the set `ValuesToLink`. I was imagining that IRLinker could link `*_odr` globals and change their linkage to `available_externally` if they were not members of `ValuesToLink`, similar to how it is already handling `available_externally` globals.
> > I was also surprised to see that code in IRMover that always links available_externally, 
> 
> I personally *hate* this kind of behavior that exhibit magic hidden from the API. This makes the software component harder to user and reason about, and invariant unclear. 
> 
> I mentioned it multiple times in the case of the IRMover during ThinLTO bring-up that I'd even consider this a layer violation in term of responsibility of the IRMover (which should be moving the IR we tell it to and nothing else, *deciding* what to move being the other layer.).
Fair point, it sounds like the consensus is in favour of doing everything explicitly in the LTO layer then.

I sent out D29435 which moves the available_externally special casing to clients and rebased this change on top of it.

https://reviews.llvm.org/D29367