@steelannelida hi, would you like to give me a reproducer?

Thanks for finding this! We didn't notice this before.

In D146543#4217194, @weiwang wrote:

In D146543#4215777, @ChuanqiXu wrote:

Does your downstream patch address the same issue? Maybe we can consolidate here.

Oh, not exactly. It looks like you're saving the debug information under O0. While we're trying to save the debug information within optimization, which might not be so natural. And it looks like the problem occurs within opaque pointer and our downstream compiler didn't start to use opaque pointer yet. So in another word, this should be regression bug if I understand things correctly.

Yeah, we generally use -O0 for better debugging experiences. The issue was first reported from our internal release, which is based on clang-12, so I think it exists before and after opaque pointer. My plan is to fix upstream and then backport.

LGTM now.

Does your downstream patch address the same issue? Maybe we can consolidate here.

Yeah, I feel the current patch is good/innocent personally. But I agree it is pretty frustrating to do duplicated work. So I would love to wait for your P1815 patch to make the decision later.

In rG1e0709167f5edd330889f51bb203c458bdb5e359#1186995, @krasimir wrote:

In rG1e0709167f5edd330889f51bb203c458bdb5e359#1183439, @krasimir wrote:

In rG1e0709167f5edd330889f51bb203c458bdb5e359#1182790, @ChuanqiXu wrote:

It will be helpful if you can provide a reproducer or a reproducer command.

We're looking into this. We are hitting this in a large modules build and it's taking up some effort to reduce.

@ChuanqiXu I tried to reduce the error but it seems to only reproduce using a very large build graph with several large modules interacting. I made some progress, but at this rate it's taking a very long time to reduce further, so it's unclear if the issue is really with this patch or something with the code itself. At this point I have no objection to this patch.

LGTM. Thanks.

In fact, we did similar things in the downstream too. But we didn't contribute it since we feel it might not be so good. Since it is a little bit dirty and it is natural that the debug information is missing. If you still want to do this, I feel CoroCloner::salvageDebugInfo() may be a better place.

LGTM. Thanks.

Not at the moment. There are some CI issues not related to modules which should be resolved first. However I am testing with the CI configuration locally. I'm still fixing bugs in the modules and investigating issue which might be Clang bugs.

Okay, that pretty much paints us into a corner, I guess. If we don't define it any longer, we break existing (working) uses of the feature on Windows, but we defined it prematurely. In this case, let's leave the macro defined so we don't break existing uses -- in the future, I think we should be more conservative with defining feature test macros.

Add CI jobs to test modules with parts disabled.

Ugh, that does sort of change the calculus for whether we do or don't claim support on Windows. If removing the definition of that macro on Windows causes significant code breakage, that would be a reason we should leave it defined. But do we have evidence of that?

The lookup problems might be considered to be quite a serious bug, and so we should consider possible back porting and try to avoid making large changes in theses patches (once that problem is solved, then we can refactor, I think).

Should we also be updating InitPreprocessor.cpp at the same time, for non-Windows targets?

but we don't define __cpp_­impl_­coroutine: http://eel.is/c++draft/tab:cpp.predefined.ft

Address comments.

I don't remember clearly if we have a test for the initializer in the implementation unit. Ignore this if we already have one.

(I don't why I can't send emails in the original mail address. It told me that your email addressed is not recorded by MX, while I don't know what is MX. So I tried to reply your email here)

In D145639#4198837, @ilya-biryukov wrote:

In D145639#4198794, @ChuanqiXu wrote:

! In D145639#4198815, @ChuanqiXu wrote:
The RVO seems to be mandated by the standard.

Yeah, I thought so... But WG21 decided to not make it in a recent meeting... So we can only make it as a non-mandated compiler optimization.

That's unexpected. Are there any wording change proposals published?
Our understanding comes from the reading of dcl.fct.def.coroutine/7

The expression promise.get_return_object() is used to initialize the glvalue result or prvalue result object of a call to a coroutine.

And we think RVO is mandated when initializing the result objects mentioned in general.
Also, from https://github.com/llvm/llvm-project/issues/56532:

Q: if the initialization does occur later, by what mechanism the prvalue result of get_return_object is forwarded to that initialization.
A: (see below)
Case 1/2. Same type of get_return_object and coroutine return type.
Constructed in the return slot.
Case 3. Different types:
(1) Constructed temporary object prior to initial suspend initialized with a call to get_return_object()
(2) when coroutine needs to to return to the caller and needs to construct return value for the coroutine it is initialized with expiring value of the temporary obtained in step 1.

Yeah, this one should be correct. Modules techniques use *::Profile method to judge if two entities are the same extensively. So it would be best to add a test case for modules like we did in https://reviews.llvm.org/rG3e78fa860235431323aaf08c8fa922d75a7cfffa. And it doesn't matter if you are not interested, I'll take it later then.

The RVO seems to be mandated by the standard.

The reproducer seems like what we're searching for tests in https://reviews.llvm.org/D145641. Reverting is common in Clang/LLVM. But let's wait for the response from @bruno temporarily. I think it makes sense to revert this one if @bruno don't response tomorrow. And we can commit this one and D145641 quickly the next time. Recommit is common too.

In D144844#4197067, @dblaikie wrote:

I don't think of this as a performance regression for users though - this functionality's never really "shipped" so we get to choose what the baseline is.

And I think a reasonable baseline to compare to isn't this implementation we don't think is ideal (because of the build invalidation issues, if nothing else, caused by having thick PCMs) - I think the baseline is what a users non-modular code is. And in non-modular code these non-inline functions would be in the implementation files, not able to cross-TU inline without LTO.

I think not providing definitions of non-inline functions for cross-TU optimizations is not a regression, but exactly in-line with existing non-modular behavior, which is totally fine.

LGTM. Thanks.

However, "performance" also includes compilation speed in the 'no optimisation, debug' case - that is also considered very important. So, perhaps, the short-term approach should be (as @dblaikie suggested) to include the bodies for -O >= 3?

I filed another issue (https://github.com/llvm/llvm-project/issues/61427) to reflect [basic.lookup.general]p2.3. So there are multiple issues. Let's handle them in multiple patches too.

It should be "Plan Changed" instead of "Abandoned".

Got your points. Let's postpone this one.

In D145965#4194514, @iains wrote:

In D145965#4192051, @iains wrote:

I was thinking at one stage to add an 'Implementation' module Kind, but at the moment I do not think it is worth extending the size of the ModuleKind enum bit field for this (since we now have used up all the entries with the new 'ExplicitGlobalModuleFragment' case). That also has a large impact also for relatively few uses.

I have changed my mind here.
It seems that we are getting confusing decl context entries without having a proper module for the implementation (even though we will never write such a module to a PCM, since it is not importable).

I was wondering if it would be possible to avoid having separate values for PartitionInterface/Implementation (since they both have a BMI - it is only a question of whether that can be re-exported). We would have to add one bit to the PCM to deal with that, so that we might as well extend the ModuleKind enum to 4 bits.

So .. please wait for one or two days, I will try to refresh the implementation patch (D126959) and see if that resolves some of the issues I am seeing with P1815

(this does not alter the situation that you are going to revisit the isModuleUnitOfCurrentTU if that is needed - but it might not be; since having a separate module for the impl. will avoid some of the ambiguities anyway).

Additional Note: we also have a situation in the lookups for dependent names during template instantiation that the visibility of decls can need to be assessed in a different module context from the current module. So that we might also need to have a areTheseModulesFromTheSameTU(Module *A. Module *B)

In D145965#4192147, @iains wrote:

In D145965#4192072, @ChuanqiXu wrote:

In D145965#4192051, @iains wrote:

The checks for internal-linkage symbols and the improvements to diagnostics do not depend on how Sema::isModuleUnitOfCurrentTU is implemented, so that I think we could fix these problems and then deal with the refactoring later.

In either case, I do not have much time to work more on this right now.

I'll try to make it.

Perhaps then, we can split out the changes to Sema::isModuleUnitOfCurrentTU and see how much of the functionality is still working (with the intent that the full fix will come along later).

So if this is split into two

1. the fix to Sema::isModuleUnitOfCurrentTU (which you will be updating) and
2. the uses of that to fix the lookups and diagnostics?

(I can use both as a temporary fix .. but 2 could be applied with the existing isModuleUnitOfCurrentTU implementation).

In D145965#4192051, @iains wrote:

In D145965#4191973, @ChuanqiXu wrote:

Yeah, it is indeed a problem that Sema::isModuleUnitOfCurrentTU doesn't work for implementation units. And I have been thinking about this for a while. My thought is that the root cause may be that we shouldn't push the module interface to Sema::ModuleScopes() for the implementation unit. (We need some other small refactoring).

I think it is not small refactoring - we make extensive use of "getCurrentModule()" to determine if we are processing a module and the interface to know which module the implementation belongs to.

If we add the implicitly imported interface to "imports" instead of the module scope we will need to make sure that we deal with all these cases where we rely on the interface module for this information.

I was thinking at one stage to add an 'Implementation' module Kind, but at the moment I do not think it is worth extending the size of the ModuleKind enum bit field for this (since we now have used up all the entries with the new 'ExplicitGlobalModuleFragment' case). That also has a large impact also for relatively few uses.

Yeah, it is indeed a problem that Sema::isModuleUnitOfCurrentTU doesn't work for implementation units. And I have been thinking about this for a while. My thought is that the root cause may be that we shouldn't push the module interface to Sema::ModuleScopes() for the implementation unit. (We need some other small refactoring). I feel this is more natural and consistent. I thought to take this one but we didn't use implementation units in the downstream and there is no related issue reports. So I didn't start to work on it... If you are not hurry, I'd like to take this. Otherwise I'd suggest you to try the above method I mentioned.

@iains @dblaikie gentle ping~

LGTM.

It looks like that the Create template functions get merged expectedly. And the dummy variable shouldn't get merged since it is not in a mergeable primary context (function context is not a mergeable primary context) (see https://github.com/llvm/llvm-project/blob/23bd0e037b744d1f93bdfad59b7575017725a96c/clang/lib/Serialization/ASTReaderDecl.cpp#L3107-L3151).

It will be helpful if you can provide a reproducer or a reproducer command.

LGTM. Thanks!

LGTM. Thanks! BTW, in such cases, you can commandeer the revision generally.

This is basically a reverting of https://reviews.llvm.org/D117087. So it should be good according to our previous talk.

Is there any new progress on this?

One of the issues in libc++ is that vendors can decide to disable filesystem header. Since parts of that library are in the dylib the std module can't provide what's in filesystem. Does your method also have that feature?

LGTM.

LGTM.

About distributing the std modules, this is what we do in the downstream:

BTW, I am also curious that if the current phab CI runs the std_modules test now? If yes, it is pretty good.

Deprecated since we prefer D144994

I noticed the test lives under std_modules dir. But they should live under std dir finally, right?