[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#1297809, @pcc wrote:

> In https://reviews.llvm.org/D53890#1297566, @tejohnson wrote:
>
> > 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?
>
>
> That sounds like it would be simpler. We could present a choice between "use summary-based WPD" and "use traditional WPD", where the default is to use summary-based WPD, and record that in a flag. Then we wouldn't need to change the definition of an LTO unit, and I think we would only need to track whether that flag is inconsistent.


How about I then add optional argument to -fwhole-program-vtables. I.e. "=index" and "=ir" (not sure those are the best names, what do you suggest?), and by default -fwhole-program-vtables would be the same as "-fwhole-program-vtables=index". Although we'd probably want to stage the changes so that the initial default is "=ir", so that we can update internal uses of the option, then flip the default. Eventually, -flto=thin would imply -fwhole-program-vtables=index.

CodeGenOpts.LTOUnit stays the same (default will stay on, controls insertion of type metadata)
CodeGenOpts.WholeProgramVTables stays a bool (enabled if any flavor of -fwhole-program-vtables is enabled, controls insertion of type test etc intrinsics)
Add a new CodeGenOpts.SplitLTOUnit bool which would only be set if CFI is enabled or if -fwhole-program-vtables=ir is specified. It will control whether the module is split in the presence of type metadata.

We then also record whether the new SplitLTOUnit is set in the index to detect mismatches during the thin link.



================
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
----------------
pcc wrote:
> 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.
I think I can fix this by flipping the sense of the new flag - i.e. set the flag to true in the (soon to be default) non-split case but unset in the (currently default) SplitLTOUnit case.


Repository:
  rL LLVM

https://reviews.llvm.org/D53890