[clang] [C++20] [Modules] Introduce reduced BMI (PR #75894)

[Chuanqi Xu via cfe-commits]

ChuanqiXu9 wrote:

> @ChuanqiXu9 very sorry for the slow review. It would help me if the design was described in the commit message instead of trying to deduce it from the patch (maybe it's in a thread somewhere - so a cross-reference would help).

hi @iains , sorry for the confusion. It may be hard to balance how detail the message it is. So it is no problem at all to ask questions as long as it need. This should be the point of the review process too.

For this patch itself, the design is to introduce a  CC1 command line `-emit-reduced-module-interface` and the corrresponding action `GenerateReducedModuleInterfaceAction`. Both of them are aimed to help testing reduced BMI instead of being used by end users actually. Then it might be straightforward to get it by seeing the implementation of `GenerateReducedModuleInterfaceAction`.

Like I said in the commit message, this patch itself doesn't involve anything relevant to user interfaces. I left it to the latter patches.

> 
> two immediate questions and one observation:
> 
> * I see you are using a multiplex consumer (actually, for some reason, I thought you were objecting to that part of my design); 

In fact, I am not objecting your design due to multiplex consumer. I am objecting your design in implementing it in `FrontendAction::CreateWrappedASTConsumer()`. I think this is not the correct place to introduce language or module specific things.

> does this mean that your proposed solution can emit both the object and the reduced BMI from a single compilation job?

Yes, this is the goal. I've already had a patch in the downstream to do that. But I am still wondering if I can implement it better.

> * I was concerned from earlier conversations that this design might require a codegen back end to be instantiated to allow the reduced BMI (which would be bad for --precompile/-fmodule-only type jobs). Any comments?

I am not sure if I understand this. What does it mean "require a codegen back end to be instantiated to allow the reduced BMI "? Do you mean to not touch CodeGen part or to not touch the CodeGen action? My local patch touched the code gen action without touching any real CodeGen related things. 

>  the reduced BMI (which would be bad for --precompile/-fmodule-only type jobs)

For `--precompile/-fmodule-only` type jobs, I'll create another action to make it (Similar to existing `GenerateModuleInterfaceAction`). Then both of the actions will try to reuse the same consumer `ReducedBMIGenerator` to avoid repeated works.

> * It would be better to avoid introducing more layering violations but, as we discussed in face-to-face meetings, I have less concern on the output side.  It still seems to me that the best model is one where we have AST transforms (that very likely need Sema to be correct) and then the serializer is a simple as possible.

Yeah, it should be less concerned. BTW, currently the simpler serializer/deserializer should be ASTRecordWriter/ASTRecordReader. And the current ASTReader/ASTWriter takes some semantical job.

> 
> so something like
> 
> ```
> raw AST +======== > codegen (when required)
>         |
>         + =====> AST transforms ====> BMI output.
> ```
> 
> As I understand the patch you are combining the transform with the output?

On the one hand, the **current** patch doesn't do that. The current patch is almost a NFC patch. It belongs to the following patch. On the other hand, the answer may be yes. Probably I did the `AST transforms` you said in the AST Writer. I don't feel it is so awful.

---

Given all of us loves reduced BMI, I suggest we can focus on current patch then discuss user interfaces related things in the next patch after this got landed.



https://github.com/llvm/llvm-project/pull/75894