Page MenuHomePhabricator

[C++20][Modules] Introduce an implementation module.
Needs ReviewPublic

Authored by iains on Jun 3 2022, 5:15 AM.

Details

Summary

During code-gen (for initializers) we need to process module implementation
units differently from interfaces. At present, this is not possible since
the interface module is loaded as representing the "current module scope".

We cannot treat an implementation unit as if it was a regular source, since we
need to track module purview and associate definitions with the module.

The solution here is to create a module type for the implementation which
imports the interface per C++20:
[module.unit/8] 'A module-declaration that contains neither an export-keyword
nor a module-partition implicitly imports the primary module interface unit of
the module as if by a module-import-declaration.

Implementation modules are never serialized (-emit-module-interface for an
implementation unit is diagnosed and rejected).

Diff Detail

Event Timeline

iains created this revision.Jun 3 2022, 5:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2022, 5:15 AM
iains added a subscriber: Restricted Project.
iains published this revision for review.Jun 3 2022, 5:28 AM

we need to distinguish implementation and interface in code-gen.

Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2022, 5:28 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Implementation modules are never serialized (-emit-module-interface for an implementation unit is diagnosed and rejected).

Never? Or just not via -emit-module-interface? I would expect to be able to serialize via -emit-ast.

iains added a comment.Jun 3 2022, 9:02 AM

Implementation modules are never serialized (-emit-module-interface for an implementation unit is diagnosed and rejected).

Never? Or just not via -emit-module-interface? I would expect to be able to serialize via -emit-ast.

My current intent is that this is a implementation detail (a mechanism for maintaining the module purview and type for Sema and CG).

As things stand, I do not think we are set up to handle (de-)serialization of two kinds of module with the same name, so I'd prefer to defer
possible extension to allow this for now (not against the principle, just trying avoid a rabbit hole that's tangential to the current purpose).

ChuanqiXu added inline comments.Jun 5 2022, 8:17 PM
clang/lib/Lex/ModuleMap.cpp
910

The implementation looks similar to createModuleForInterfaceUnit really. It looks better to refactor it to createModuleUnits (or createModuleForCXX20Modules) which containing an additional argument Module::ModuleKind. It would optimize the case for partitions too, which uses createModuleForInterfaceUnit now and it is a little bit odd.

clang/lib/Sema/SemaModule.cpp
305

I feel like the 3 comments are redundant.

343–347

We could move this logic to the place Import is created.

357–358

Is it necessary/helpful to return the import declaration? Although there is a FIXME, I think the current method looks a little bit confusing.

Implementation modules are never serialized (-emit-module-interface for an implementation unit is diagnosed and rejected).

Never? Or just not via -emit-module-interface? I would expect to be able to serialize via -emit-ast.

My current intent is that this is a implementation detail (a mechanism for maintaining the module purview and type for Sema and CG).

As things stand, I do not think we are set up to handle (de-)serialization of two kinds of module with the same name, so I'd prefer to defer
possible extension to allow this for now (not against the principle, just trying avoid a rabbit hole that's tangential to the current purpose).

I think Iain's "(de-)serialization" means "generating PCM files". And it makes no sense to block users to use emit-ast for implementation units. I don't think we have divergence here since I don't find codes which try to emit an error if we are attempting to generate a PCM file for an implementation unit.


I feel better if we could add some tests:

  • As mentioned above, emit an error when we trying to generate a PCM file for an implementation unit.
  • Currently, when we use implementation units, we need to specify -fmodule-file=/path/to/the/pcm/of/primary/interface in the command line. It looks like we could omit it after the patch (as long as the PCM file of primary interface lives in the path specified by -fprebuilt-module-path)
clang/lib/Frontend/FrontendActions.cpp
835

Since ModuleKindName() here is used as a helpful for printers, it looks odd to contain the should never be serialized part. I think we should check this when we try to generate PCM files for ModuleImplementationUnit.

iains planned changes to this revision.Jun 6 2022, 2:42 AM

thanks for the reviews, I need to figure out why the implementation passes clang tests but gives fails for clangd.

iains updated this revision to Diff 434774.Jun 7 2022, 4:33 AM

rebased and tidied (still changes planned).

urnathan resigned from this revision.Aug 16 2022, 12:21 PM