[PATCH] D53890: [LTO] Record LTOUnit flag in index and validate during LTO link

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In https://reviews.llvm.org/D53890#1297258, @tejohnson wrote:

> In https://reviews.llvm.org/D53890#1293818, @pcc wrote:
>
> > > without LTOUnit (for purely summary-based class hierarchy analyses).
> >
> > We might want a different name for the flag then, because a translation unit that is subject to purely summary-based class hierarchy analysis is still part of the LTO unit, it just participates in fewer optimizations.
>
>
> I think I am confused then about the terminology. My understanding was that the LTO unit was the part that would be linked with regular LTO. Does a linkage unit's LTO unit include both the regular and thin LTO modules?


The LTO unit is a user-facing concept, and is (currently) defined as follows in LTOVisibility.rst:

> a linkage ​unit's *LTO unit* is the subset of the linkage unit that is linked together ​using link-time optimization

The splitting of a translation unit into regular and thin LTO modules is an implementation detail that allows the LTO unit features described in that document to work. So I suppose that it would be accurate to say that both the regular and thin LTO modules are part of the LTO unit, but that's just a consequence of how things are implemented. Hope that helps.

> If so, what name would you suggest?

On second thoughts, it does seem reasonable for the flag to be called LTOUnit. This reflects that the translation unit is part of the LTO unit.

To go over the cases, we will have:

1. Regular LTO, part of the LTO unit
2. ThinLTO, "traditional" WPD, part of the LTO unit
3. ThinLTO, not part of the LTO unit

and eventually:

4. ThinLTO, summary-based WPD, part of the LTO unit

So it seems like you will want to add another flag to writeThinLTOBitcode when you add case 4.

By the way, how are you planning to handle the case where a translation unit compiled for summary-based WPD is linked against a translation unit compiled for traditional WPD?



================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5247
         TheIndex.setSkipModuleByDistributedBackend();
+      // 1 bit: LTOUnit flag.
+      // Set on per module indexes. It is up to the client to validate
----------------
I think we will need to do something about this flag for compatibility. Right now all translation units are part of the LTO unit. So that means that once this change goes in we will be unable to link old bitcode with new bitcode. We might need a way of distinguishing old bitcode from new bitcode so that we can ignore the LTO unit flag in old bitcode.


Repository:
  rL LLVM

https://reviews.llvm.org/D53890