[PATCH] D134267: [C++] [Modules] Support one phase compilation model for named modules

Tue Oct 11 20:52:44 PDT 2022

ChuanqiXu added a reviewer: tschuett.
ChuanqiXu added a comment.

In D134267#3849629 <https://reviews.llvm.org/D134267#3849629>, @dblaikie wrote:

> To the original point I was making about implicit modules (which might've been my own confusion due to the term being brought up in D134269 <https://reviews.llvm.org/D134269>):
>
> Implicit modules is the situation where the compiler, finding that it needs a module while compiling some usage, knows how to go out and spawn a new process to create that module and then cache it. The build system never knows about modules, their builds or their dependencies.
>
> This patch had some superficial similarities - specifically around having an on-disk cache of modules that the user/build system/etc invoking the compiler isn't necessarily aware of/managing/invalidating/observing/etc. But it does differ in an important way from implicit modules in that the compiler won't implicitly build modules - you still have to specify the modules and their usage as separate compilations in-order (ie: modules need to be explicitly built before their usage). I think that makes a big difference to this being feasible, at least in the small scale.

Oh, now I got your point. It is caused by the imprecise name. My bad.

> The remaining concern is that this feature should likely not be used by a build system - because it won't know the dependencies (or, if it does know the dependencies then the build system, not the compiler, should be managing the BMIs) & so won't know how to schedule things for maximum parallelism without incorrect ordering, and correct rebuilding of dependencies when necessary.

I agree it won't reach the maximum parallelism. But I think it should be able to rebuild correctly if the build system understands the dependencies between module unit. For example, if B.cpp imports module A, and A is defined in A.cppm. And when A.cppm changes, it will be fine if the build system will compile A.cppm first and compile B.cpp then. I think this is achievable by the build system. (For example, the P1689 <https://reviews.llvm.org/P1689> proposal I'm working on). So the problem becomes a performance problem instead of a correctness problem. So it looks not bad to me. I still feel it is not good to make perfect as the enemy of better.

P.S: there is another case: after we built the program and we remove A.pcm without touching A.cppm. Then the current model can't hold it unless the build system can recognize the pcm actually. But this case won't make me too uncomfortable.

> In any case, it looks like there's design discussions to be had? Not sure if this is the right place to have them, but maybe it is. It might be easier to discuss them on discourse with hopefully some relatively narrow examples with command lines shown, etc. (as already some of the conversation seems to be fragmented between this change and the doc change)

Yeah, we can discuss it somewhere else.

@tschuett

> What really worries me that you are talking about one phase compilation and there is a module cache. Even if it is one phase compilation you must pass all dependent modules on the command line.

I thought the compiler can search in the module cache automatically. Do you mean the style is bad?

> The one phase compilation should work with -fno-implicit-modules!

Great catch! I think we can solve the problem by another option (`-fcxx-modules-cache-path` ?) to decouple with clang modules. Originally I feel it may be better to reuse the option to avoid confusion. But it looks like there are too much logics about modules cache in the clang modules.

> I do not think this patch fully addresses the issue of harmonising GCC and clang's command lines for modular builds (because it does not deal with discovery of modular code in 'normally named' sources), and my investigation of trying to do this in the driver suggests that it would be much more complex than doing it in the FE.

Yeah, it is not sufficient to say this patch addresses the command line conformance issue. This patch only ease the use of modules.

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

https://reviews.llvm.org/D134267