[PATCH] D23488: ThinLTO: add early "dead-stripping" on the Index

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: include/llvm/LTO/LTO.h:389
+    /// Keep track if the symbol was referenced by the regular LTO partition.
+    bool VisibleToRegularLTOPartition = false;
+
----------------
tejohnson wrote:
> mehdi_amini wrote:
> > tejohnson wrote:
> > > mehdi_amini wrote:
> > > > tejohnson wrote:
> > > > > pcc wrote:
> > > > > > mehdi_amini wrote:
> > > > > > > pcc wrote:
> > > > > > > > Don't you just need one flag: VisibleOutsideThinLTO (or something).
> > > > > > > I think one flag should be enough, but shouldn't we have the same flag for LTO internalization?
> > > > > > > 
> > > > > > Wouldn't that be redundant with Partition == 0?
> > > > > You're right, we only need one flag (forgot that the VisibleToRegularObj flag was added to GlobalResolutions for this patch). I will combine them as suggested.
> > > > You're right! 
> > > > So do we need to record every ThinLTO module as an individual partition? When do we use this?
> > > > We could have `enum partition { Unkown, Global, LTO, ThinLTO }`?
> > > > So do we need to record every ThinLTO module as an individual partition? When do we use this?
> > > 
> > > To detect when symbols are used by multiple ThinLTO modules (i.e. exported). E.g. it transitions from Unknown -> partition1  when first called with Partition=partition1, then from partition1 -> External if invoked again for the same symbol with a different partition2.
> > > To detect when symbols are used by multiple ThinLTO modules (i.e. exported). E.g. it transitions from Unknown -> partition1 when first called with Partition=partition1, then from partition1 -> External if invoked again for the same symbol with a different partition2.
> > 
> > The way partitions are handled and the flag VisibleOutsideThinLTO is still not clear to me, we know when symbols are exported from the index itself why do we need partitions for that?
> > 
> > 
> > The way partitions are handled and the flag VisibleOutsideThinLTO is still not clear to me, we know when symbols are exported from the index itself why do we need partitions for that?
> 
> At some point we have to collate that information to be able to look up which guid are exported from their modules (for creating the ExportedGUIDs set for internalization). We could detect cross module references among ThinLTO modules by walking the index, but since we have the information here already, it seems more efficient to just keep track of it. Otherwise we need to walk the whole index, comparing module paths, etc.
> At some point we have to collate that information to be able to look up which guid are exported from their modules (for creating the ExportedGUIDs set for internalization). We could detect cross module references among ThinLTO modules by walking the index, but since we have the information here already, it seems more efficient to just keep track of it. Otherwise we need to walk the whole index, comparing module paths, etc.

Do we have an example that would cover the following case:

```
// main.cpp
void bar1();
void bar2();
void baz();
void foo1() {
  bar1();
}
void foo2() {
  bar2();
}
int main() {
  baz()
}
````

And 

````
// bar.cpp
void bar1() {
}
void bar2() {
  foo2(); // The cycle shouldn't prevent dead-stripping.
}
void baz() {
   // Adding a call to bar() here should not prevent internalization.
}
````

With only main exported by the linker, I expect us to be able to:

1) dead-strip from the index foo() and bar()
2) "internalize" (drop) foo() and bar() eagerly from the IR

I think the current logic should be OK. But I still find the partition thing and the `ExportedGUIDs` confusing.


================
Comment at: lib/LTO/LTO.cpp:879
           Res.second.Partition == GlobalResolution::External)
         ExportedGUIDs.insert(GlobalValue::getGUID(Res.second.IRName));
     }
----------------
Could we *not* add DeadSymbols to `ExportedGUIDs` here and remove the `DeadSymbols.count(GUID)` from the test below?


https://reviews.llvm.org/D23488