tejohnson added a comment. In https://reviews.llvm.org/D53524#1276038, @pcc wrote:
> 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. Just to clarify, since there is already a -flto-unit flag: it is currently a cc1 flag, did you want it made into a driver option as well? > > >>> 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. Ok, let me work on that now and we can get that in before this one. 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
