pcc added a comment. In https://reviews.llvm.org/D53524#1274505, @tejohnson wrote:
> In https://reviews.llvm.org/D53524#1271387, @tejohnson wrote: > > > In https://reviews.llvm.org/D53524#1271357, @pcc wrote: > > > > > The reason why LTO unit is always enabled is so that you can link > > > translation units compiled with `-fsanitize=cfi` and/or > > > `-fwhole-program-vtables` against translation units compiled without > > > CFI/WPD. With this change we will see miscompiles in the translation > > > units compiled with CFI/WPD if they use vtables in the translation units > > > compiled without CFI/WPD. If we really need this option I think it should > > > be an opt out. > > > > > > Is there an important use case for support thing mixing and matching? The > > issue is that it comes at a cost to all ThinLTO compiles for codes with > > vtables by requiring them all to process IR during the thin link. > > > Ping on the question of why this mode needs to be default. If it was a matter > of a few percent overhead that would be one thing, but we're talking a *huge* > overhead (as noted off-patch for my app I'm seeing >20x thin link time > currently, and with improvements to the hashing to always get successful > splitting we could potentially get down to closer to 2x - still a big > overhead). This kind of overhead should be opt-in. The average ThinLTO user > is not going to realize they are paying a big overhead because CFI is always > pre-enabled. Well, the intent was always that the overhead would be minimal, which is why things are set up the way that they are. But it doesn't sound like anyone is going to have the time to fully address the performance problems that you've seen any time soon, so maybe it would be fine to introduce the -flto-unit flag. I guess we can always change the flag so that it has no effect if/when the performance problem is addressed. >> Can we detect that TUs compiled with -flto-unit are being mixed with those >> not built without -flto-unit at the thin link time and issue an error? > > This would be doable pretty easily. E.g. add a flag at the index level that > the module would have been split but wasn't. Users who get the error and want > to support always-enabled CFI could opt in via -flto-unit. Yes. I don't think we should make a change like this unless there is something like that in place, though. The documentation (LTOVisibility.rst) needs to be updated too. Repository: rC Clang https://reviews.llvm.org/D53524 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
