Looks great, thanks!

Looks great!! Thank you for also updating the other call sites. I had a few additional comments, but once those are taken care of I can accept. (When I do, will you need me to merge this commit on your behalf?)

I think I'll commit this later today, but let me know if you have any thoughts on adding an interface for indenting text to the printer, @rriddle. I'd be happy to try my hand at sending a patch to do so, as well.

Not at all, thank you for all the great feedback!

Two small tweaks: give the BlockRange caseDestinations parameter of the op builder a default empty value, and "indent" cases before printing them.

Fix comment typo in test.

Thanks for the integer and newline patches @rriddle! I've updated this patch to use those new features. And thanks for another round of reviews @ftynse, I think I've responded to all the feedback given so far, let me know if I've missed anything.

Sounds good, thanks! I agree, I'll modify this patch later today such that the MLIR llvm.switch op accepts zero cases, just as LLVM IR switch instruction does.

Rebase past D92845, add instantiated SwitchInst to the branch mapping.

Thanks for answering all my questions! I think I've responded to all of your feedback, let me know if I missed anything. I also have a question about parsing and printing integers that I'll post as an inline comment.

So, unfortunately I don't think it's feasible to use the declarative assembly format for this op, at least not without making some TableGen modifications to pass custom directives a reference to the OperationState. I'm also not certain it's possible to get rid of case_operand_offsets in favor of operand_segment_sizes, since this op seems unique in storing a variable list of variadic operands. But, aside from those two points, I think I've addressed all of your review comments so far. Thank you, @ftynse and @rriddle! Please take another look at this when you have some time.

Add to the test for operand segment sizes.

Oops, forgot to clang-format.

I'm adding an unrelated change to the AttrSizedOperandSegments verifier that prints how many elements it found in operand_segment_sizes. It's not necessarily part of my patch, but it helped me when debugging. I'd be happy to split it out into its own if anyone would prefer that.

Thanks for the review, @ftynse!

No problem, thanks for this!

By the way, I tried to accept this diff and leave the following inline comment on SemaExpr.cpp:15799:

This LGTM! Looks like the last time this variable was touched was in 2010 as part of a mechanical renaming, https://github.com/llvm/llvm-project/commit/a771f46c82d7. At that point it was used, as a parameter to the static function MakeObjCStringLiteralFixItHint. That static function was later renamed on Dec 17 2013 https://github.com/llvm/llvm-project/commit/bd714e9bb12, to ConversionToObjCStringLiteralCheck, and on the following day Dec 18 2013 in https://github.com/llvm/llvm-project/commit/283bf895066 the signature was changed to no longer take a FixitHint parameter. The author must not have removed this variable at that point because technically it is still referenced, but as you point out, it's never mutated in a way that gives the hint a value.

Thanks for the fix!

Excellent, thanks for this!

Thanks, this looks good to me!

Sweet! Thanks for the reviews/responses, LGTM :)

I don't have a preference as to whether Sema::ActOnCoroutineBodyStart returns true or false to indicate failure, although I think returning true for failures is a bit of a pre-existing convention, such as here, where true is returned for an invalid declaration: https://github.com/llvm/llvm-project/blob/84167a8d58e8af79625abcffdf2c860d556955e6/clang/lib/Sema/SemaDecl.cpp#L1572

Excellent, thank you! The test failures on the diff appear to be legitimate, they reproduce for me when I apply this patch to my local checkout and run ninja check-clang. Could you take a look?

Of course, your approval is very welcome! 😄 I'll go ahead and land this today, thanks for the review!

LGTM, thanks!

Yup, makes sense. In the codebase I maintain, I think there are a few distinct coverage issues, so I'll keep this patch applied to my fork of LLVM for the time being, until I send patches to fix those issues. But it sounds like this isn't something that we'd want in LLVM trunk, so I'll close this out. Thanks!

Looks great, thanks!

Looks good to me! Initially I was wondering about whether shouldBeMustTail ought to check whether the next instruction is a ret, or a bitcast that's used by a ret, but I see simplifyTerminatorLeadingToRet takes care of that check. Thanks for the patch!

Excellent, thank you! I had one question, otherwise this looks great!

I see. No, I think whenever we have malformed coverage data, it points to an underlying bug in the compiler. Still, I think llvm-cov's treatment of invalid data is too severe. Is there a good reason it should bail entirely? If it's able to process all but a single coverage region, it seems like it ought to report what it was able to process, no? I think that at least having the option for llvm-cov to behave this way is helpful, although I can understand if others disagree.

Awesome, thank you!

I may be wrong, but I believe this breaks check-llvm when run with the following configuration: cmake -DLLVM_BINUTILS_INCDIR=/path/to/binutils/include:

Alternatively, I considered modifying the clang::OverloadCandidate constructor to initialize IsSurrogate with a false value. I feel doing so would be safer, but I stuck with what appears to be the convention: initializing OverloadCandidate members are the site of use.

Nice, thanks for this improvement! Am I right in thinking patch is dependent upon D76119? My understanding is that, without that patch, then we wouldn't be able to rely on lifetime intrinsics when using -O0. You may want to use Phabricator's "Edit Related Objects" link to specify that this patch depends on that one.

Nice, thank you. I like the test case you added, it feels more reduced than the one in D76118 -- even if it does use long type names such as awaitable, they make the test case easy to understand. But I had a suggestion to expand upon the test case that I think would help this patch.

Awesome, thank you! This is an issue I've been running into as well in an internal codebase, I'll check whether this patch addresses it.

Friendly ping! OverloadCandidate has uninitialized members and so can cause UB if used incorrectly in another part of the compiler, so I feel this is a fairly straightforward patch: prevent UB by initializing all members. But I'd like some review to confirm. @rsmith? @aaron.ballman?

Friendly ping! New PM guys, maybe @chandlerc or @lebedev.ri could review?

yep, I have tried the two samples, same behavior as clang6.

Thanks again for the review. I moved findDbgDeclareUses to Local.h (no unittest for now but I'll add one in a follow-up diff for both it and FindDbgAddrUses, which doesn't have one either) and I removed the redundant loop. I'll commit this now and add an lldb regression test in a follow-up patch.

Thanks for the review, @jmorse! I responded to your comments: I delete only dbg.declare, replace other uses with undef, and I removed unnecessary attributes.

They are not broken [1], but you may need to add libcxx and friends to your list of projects in your LLVM cmake invocation.

Thank you for this patch, I'll give it a proper review tonight. But in the meantime, I wonder if you've tried this with any of the sample programs for this bug report, https://bugs.llvm.org/show_bug.cgi?id=40656 ?

Add checks for !dbg locations.

I tried adding an lldb test for this, but I encountered 39 failing tests when I ran ninja cxx check-lldb, and it made me wonder whether tests that use libc++ are currently broken?

Nice catch, thanks!

As a side note: Are you using LLDB as your debugger? We could probably use a couple of end-to-end tests in the LLDB testsuite for coroutines as well if you are interested in contributing one.

Remove Git commit hashes from the debug info metadata, just to slim it down a tiny bit more.

I played around a bit more and got it down to just 13 nodes. I think this is the bare minimum.

Wow, great suggestion, @dstenb! That brought the number of nodes down to 119 -- a reduction of 82%! (Also, I ran clang-format on the include files to appease the pre-merge checkers -- although to be honest, I'm not sure I agree with the rationale here, because this diff didn't modify the include that clang-format is complaining about. But oh well.)

Sorry this review took so long, and thank you for addressing the regression I introduced in D43242!

Friendly ping for reviews on this one. I've found this to be a great tool for debugging the passes running during ThinLTO.

This looks good to me, it sounds like all of @rnk's comments were addressed, and I'd like to unblock D74516 that's stacked on top of this, so I'm going to accept. @amonshiz do you want me to commit this for you? (And @rnk, any comments before I do?)