[PATCH] D144844: [C++20] [Modules] Offer -fno-import-inter-module-function-defs to avoid duplicated compilation in modules

Tue Mar 14 13:26:37 PDT 2023

dblaikie added a comment.

In D144844#4193727 <https://reviews.llvm.org/D144844#4193727>, @iains wrote:

> In D144844#4193568 <https://reviews.llvm.org/D144844#4193568>, @dblaikie wrote:
>
>> Seem to recall @iains and others were concerned about the number of modules flags - this one I'd be inclined to suggest we shouldn't add if possible. If one way or the other is generally better - we should, at least initially, dictate that behavior entirely until someone can demonstrate divergent use cases that seem reasonable to support but must have different behavior here.
>
> Agreed, [IMO, at least] we have a very large set of modules options, and expanding that should only be done if no sensible alternative can be found (we are already seeing users getting confused about which ones apply in each case).
>
> A second point here - that (once we have the ability to generate object and PCM in the same compilation) that we will move to elide the function bodies for non-inline and non-dependent cases from the PCM, so that this problem will magically "go away" (to restore the current behaviour, we'd then be using some optimisation control to avoid the elision, I suppose).

Yeah, this seems like the simplest concrete point to suggest we just change the defaults here - that's the plan moving forward, and I don't think it's worth trying to maintain two codepaths/supports here - people can write the functions as explicitly `inline` when they want cross-module inlining & we can let non-inline functions be the same as they were before modules.

If, years from now, someone wants to prototype some stronger optimization opportunities in modules - let's do that later/then?

>> The performance of cross-module inlining could be achieved with something like ThinLTO if we were to lean in favor of not allowing cross-module inlining by default, for instance.
>
> +1 this seems exactly what LTO is intended for (also there are folks who seem to have an expectation that the BMI somehow magically contains an optimised representation of the source - which again is the province of LTO).

There's some advantages (build system complexity, time, etc) to doing it at compile-time, rather than link-time, but I don't think we have enough data to be worth the complexity for now - I'd vote for letting the feature go/removing it for now, and folks can try to bring it back with data/measurements later.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144844