@iains Do you agree to submit this one first? Since all the discussions/questions I see in this page now it not about the patch itself. The patch itself should be good personally I thought.

Prefer https://reviews.llvm.org/D126189

In D126277#3540094, @jmorse wrote:

what scenarios is salvageDebugInfo called in? I'm trying to reason about when the isComplex test might be necessary.

Address comments.

We should generate a patch with option -U9999 to offer more context to review.

Address comments.

I think I have a preference to move it to CodeGenCXX anyway however, since we're actually testing the code-generated output (this is not novel, we DO often use CodeGen tests to make sure proper overloads/etc get called).

Address comments.

Address comments.

In D119544#3527556, @erichkeane wrote:

In D119544#3526810, @ChuanqiXu wrote:

In D119544#3494281, @erichkeane wrote:

Updated to include the fix for the libcxx issue, which was that we weren't properly differentiating between 'friend' functions based on concepts. I'll likely be submitting this early monday or so, but would like to give @ChuanqiXu a chance to check out the changes.

This is the same as my 'last' committed version of this patch, plus the changes I split out into here for easier review: https://reviews.llvm.org/D125020

Sorry that I missed the ping. The change in that revision looks good to me.

I THINK what we want to do instead of trying to 're setup' the template arguments (and instead of just 'keeping' the uninstantiated expression) is to run some level of 'tree-transform' that updates the names of the template parameters (including depths/etc), but without doing lookup. This I think would fix our 'friend function' issue, and our 'comparing constraints' stuff work correctly as well (rather than storing an instantiated version, but not reusing it except for comparing them).

I agree this should be a better direction.

I actually got a response from Richard who seems to be more in favor of the solution I tried initially (the one in this patch!). The problems I have with it I think get solved by the 'friend function' rules that I pasted above, so I THINK I can fix those and be ok. I'll still need SOME level of tree-transform, but only to see if it depends on the enclosing template.

In D119544#3494281, @erichkeane wrote:

Updated to include the fix for the libcxx issue, which was that we weren't properly differentiating between 'friend' functions based on concepts. I'll likely be submitting this early monday or so, but would like to give @ChuanqiXu a chance to check out the changes.

This is the same as my 'last' committed version of this patch, plus the changes I split out into here for easier review: https://reviews.llvm.org/D125020

@rsmith gentle ping~

Cleanup codes.

Address comments:

• Lowering the introduced intrinsic in CodeGen pipelines.

In D125291#3523818, @efriedma wrote:

I don't really understand how this is supposed to interact with D125292; even if you strip the readnone attribute from the call instruction, we'll still treat a call to the intrinsic as readnone.

In D125292#3523819, @efriedma wrote:

I don't think we reached a consensus this is the right approach; please ping the Discourse thread.

I think it is better to add some tests at least.

In D125541#3518571, @urnathan wrote:

Please clarify. This is fixing a bug where the CFG was being incorrectly constructed.

This is not correct. Any change to the AST node requires change to Serialization too.

What's the reason you removed me as a reviewer? I don't think I did any thing bad.

Although this one is marked (2/3) in the title, it is independent with the previous revision logically. We could review and submit this one independently.

@rjmccall @eli.friedman @jyknight gentle ping~

This one looks like a workaround for me. It tries to skip a CFG check for the operand of return_value to avoid a potential crash. The true solution should be to construct the CFG correctly.

Address comments.

Remove invalid character

Run optimization pass in CoroCleanup conditionally.

https://reviews.llvm.org/D125292

Now we prefer https://reviews.llvm.org/D125293

@rsmith gentle ping~

Code Cleanups

In D124149#3500423, @iains wrote:

sorry for being slow, concentrating on module initialisers!

this LGTM (but we can still probably make the visibility code cleaner - perhaps after we get the main functionality installed).

Abandon this one since we prefer the proposal from James in https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015/21

@aeubanks Would you like to review this one? Although we're discussing the solution in https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015, I think the change in this revision should be a pure win.

@iains ping!

Oh, I'm busy with coroutines these days. Thanks for reminding me for this. LGTM to me basically. Only comments for style left.

Address comments:

• Don't add filter for OpenMP
• Add readonly and inaccessiblememonly attribute to llvm.coro.may_change intrinsics.

Plan changes due to the previous patch changes its plan too

Emit coro_readnone in CoroEarly pass instead of frontend.

Emit coro_readnone attribute in the frontend.

Emit llvm.coro.maychange in the frontend according to the discussion in https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015

In D124504#3476585, @nikic wrote:

In D124504#3476562, @ChuanqiXu wrote:

In D124504#3476537, @nikic wrote:

How does it block your work? Tests currently still use -no-opaque-pointers to avoid breaking this mode. I think it's generally okay to drop the option, but I'm a bit unclear about the motivation in this case -- do you want to make a change that would be incompatible with typed pointers?

The motivation is that I want to insert an intrinsic for TLS variable. The whole background could be found at: https://discourse.llvm.org/t/address-thread-identification-problems-with-coroutine/62015

The signature of the intrinsic is ptr @intrinsic.name(ptr). And I must add one bitcast if I can't use opaque-pointer. It looks like opaque pointer is enabled by default now: https://github.com/llvm/llvm-project/blob/86c770346c26ce4c9abf5a5b7ab4b5bbfdcf9d78/clang/include/clang/Driver/Options.td#L5567-L5573

So I feel like it is better to remove the --no-opaque-pointers option. I could add a bitcast to workaround if this is not wanted. Maybe the word block is not suitable here.

In that case, I'd recommend inserting the bitcast for now, if it's not too much complication. Also, you might want to consider using an overloaded intrinsic, in which case a bitcast is not necessary (using an overloaded intrinsic would also allow pointers of different address spaces, not sure if that's relevant here).

In D124504#3476537, @nikic wrote:

How does it block your work? Tests currently still use -no-opaque-pointers to avoid breaking this mode. I think it's generally okay to drop the option, but I'm a bit unclear about the motivation in this case -- do you want to make a change that would be incompatible with typed pointers?

@iains gentle ping~

Now it looks good to me.

Thanks for the explanation. This fix LGTM.

In D119544#3472537, @erichkeane wrote:

Alright, a touch more: It looks like the looping through the CurContext at line 6084 finds the wrong thing in evaluating this CXXMethodDecl! The "broken" version has its CurContext as bar in the example above, the "working" versions have foo or foo2<int>. Therefore the 2nd time through the loop, those both end up on the ClassTemplateSpecializationDecl, but the broken one finds the isFileContext version.

So I don't know HOW to fix it, but it seems that the CheckFunctionConstraints probably has to change the CurContext. I suspect the instantiation of the WorkingRequires does something like this correctly.

I'm poking on/off on it today, but if @ChuanqiXu has an idea/help, it would be greatly appreciated.