[PATCH] D33615: Move summary dead stripping before regular LTO and record results in the combined summary

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: include/llvm/IR/ModuleSummaryIndexYAML.h:132
+  unsigned Linkage;
+  bool NotEligibleToImport, Live;
   std::vector<uint64_t> TypeTests;
----------------
eugenis wrote:
> pcc wrote:
> > mehdi_amini wrote:
> > > pcc wrote:
> > > > mehdi_amini wrote:
> > > > > The addition of Linkage and NotEligibleToImport are unrelated to this change right? I feel you could commit them separately now.
> > > > > (even the serialization of "Live" here isn't clear to me if it isn't something that could just be split out).
> > > > Would we be using Linkage and NotEligibleToImport for anything though? It seems a little odd to add fields here without any test coverage. (That's not to say that we shouldn't add the fields eventually, but I think it should be accompanied by tests, i.e. we should add a JSON summary dumper to llvm-lto2 or something and convert some tests over to use it.)
> > > > 
> > > > I think the serialization of Live was part of D33702, and I'd prefer it to be done as part of that change.
> > > > Would we be using Linkage and NotEligibleToImport for anything though? It seems a little odd to add fields here without any test coverage.
> > > 
> > > What are we using the Yaml serialization for? I thought it was intended to be the "primary textual way of dumping summaries"? My comment was in this context: I was seeing this change as "fix missing serialization" and nothing else.
> > > 
> > > > (That's not to say that we shouldn't add the fields eventually, but I think it should be accompanied by tests, i.e. we should add a JSON summary dumper to llvm-lto2 or something and convert some tests over to use it.)
> > > 
> > > Of course I expect a test for these, but don't we have a test for yaml dump? I was just thinking about using `opt` to generate a bitcode file with summaries and pretty-print to yaml. (haven't looked at the existing testing right now)
> > > What are we using the Yaml serialization for? I thought it was intended to be the "primary textual way of dumping summaries"?
> > 
> > That is indeed the end goal, but right now it is only used as a mechanism to test that the lowertypetests and wholeprogramdevirt passes manipulate the summary index correctly.
> > 
> > > Of course I expect a test for these, but don't we have a test for yaml dump? I was just thinking about using opt to generate a bitcode file with summaries and pretty-print to yaml. (haven't looked at the existing testing right now)
> > 
> > There's nothing right now that reads bitcode files and then dumps their summaries as YAML. We'd have to add it somewhere, e.g. to llvm-lto2.
> > There's nothing right now that reads bitcode files and then dumps their summaries as YAML. We'd have to add it somewhere, e.g. to llvm-lto2
> 
> Or llvm-bcanalyzer.
> 
> Do you want me to remove serialization of Linkage and NotEligibleToImport? It just feels strange to serialize only part of GVFlags. When this serialization is added later, it will require updating all the same tests once again.
> 
> Do you want me to remove serialization of Linkage and NotEligibleToImport?

I'm fine with leaving it in.


Repository:
  rL LLVM

https://reviews.llvm.org/D33615