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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D53890#1297383, @pcc wrote:

> 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.


Ok, I read that, but it wasn't clear to me what it meant in the context of ThinLTO and regular LTO split modules.

> 
> 
>> 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

I am confused on the difference between 3 and 4. What makes that augmented summary that enables the WPD (i.e. 4) imply that it is now part of the LTO unit, whereas currently with ThinLTO -fno-lto-unit which implies other whole-program optimizations (i.e. 3) it is not? Reading through https://clang.llvm.org/docs/LTOVisibility.html again, I guess it is because with WPD we need to talk about LTO visibility, and that can only be done in the context of an LTO unit property. So simply doing class hierarchy optimizations such as WPD we require that the ThinLTO module be defined as being part of the LTO unit?

It sounds to  me like changing the default of the -flto-unit flag might be wrong, and what we really want is a different flag that controls the actual splitting directly?

> 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?

I think that should be an error (which this patch was intended to catch, since I was thinking LTOUnit would be disabled for case 4). But as noted above, do we want a different flag to control the splitting, then record that as a flag instead?


Repository:
  rL LLVM

https://reviews.llvm.org/D53890