[PATCH] D53890: [LTO] Record whether LTOUnit splitting is enabled in index

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson marked 2 inline comments as done.
tejohnson added a comment.

In D53890#1336239 <https://reviews.llvm.org/D53890#1336239>, @dexonsmith wrote:

> In D53890#1335770 <https://reviews.llvm.org/D53890#1335770>, @pcc wrote:
>
> > This means that
> >
> >   clang -c -flto=thin foo.c
> >   [upgrade clang]
> >   clang -c -flto=thin bar.c
> >   clang foo.o bar.o
> >
> >
> > will fail unless you add `-fsplit-lto-unit` to the second invocation. Given that the alternative is that we potentially silently miscompile, maybe that's fine. It would be possible to handle this case without aborting or miscompiling by splitting `bar.o` inside the linker and treating it as if it had always been split,
>


In a distributed build context you'd also need to record something in the index to get the backend clang invocation to also split bar.o. I'm not sure it is worth doing all this.

> but given that there's a workaround maybe that's not that critical.
> 
> I would have expected this to link successfully, but disable any optimizations that we don’t have enough information to perform safely.

>From what I understand, this wouldn't just be disabling the LTO link time whole program optimizations, but also flagging it in the index and doing some lowering in the backend of type tests (@pcc is that correct?).

Is it unreasonable to expect that the user can simply add the flag indicated by the error? The downside of that though is that users may just add the flag to their build files and unnecessarily get splitting from there on out.



================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:5934
+      // If no flags record found, conservatively return false.
+      return false;
+    case BitstreamEntry::Record:
----------------
pcc wrote:
> If we weren't emitting `FS_FLAGS` into module summaries before then can't we return true here and avoid needing to invert the flag in the bitcode?
Yes I think you are right - we only had FS_FLAGS on the combined indexes before, so that should work.


================
Comment at: tools/opt/opt.cpp:106
 
+// Enable LTO unit splitting by default for consistency with behavior
+// before adding this option.
----------------
pcc wrote:
> Wouldn't it be better to fix the test cases to be explicit instead?
You mean change the default of this to be false and update tests? Yes, I can do that and you are right it probably is better as the default is changing in clang.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53890/new/

https://reviews.llvm.org/D53890