- User Since
- Feb 16 2014, 1:10 AM (320 w, 2 d)
Sat, Apr 4
Thu, Apr 2
Of course, your approval is very welcome! 😄 I'll go ahead and land this today, thanks for the review!
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!
Sat, Mar 28
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!
Thu, Mar 19
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.
Wed, Mar 18
Awesome, thank you!
Tue, Mar 17
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:
Fri, Mar 13
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.
Thu, Mar 12
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?
Wed, Mar 11
Mar 4 2020
Mar 3 2020
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.
Mar 2 2020
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 , 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 ?
Mar 1 2020
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?
Feb 28 2020
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.)
Feb 23 2020
Sorry this review took so long, and thank you for addressing the regression I introduced in D43242!
Feb 19 2020
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?)
Feb 18 2020
Thanks for the reviews! Good point about the variable casing, I changed it and am going to commit this shortly.
Feb 17 2020
Thanks for the review! Yeah, you're right. I think the comment at the top of the file explains both the legacy and new pass manager just fine, so I removed the extra comment I'd added.
Thanks for the reviews! You're right, @jdoerfert, the file header comment is still totally applicable. I put it back in.
Rebase on top of the latest version of D71902.
Updated to use a new approach: instead of a custom pipeline parser, '-enable-coroutines' now adds coroutine passes to the new pass manager's pass builder. Using this in conjunction with the 'default<O2>' pipeline parser, this patch can now run all coroutine tests in the LLVM test suite -- and they all pass.
Thanks for the reviews, @jdoerfert! I addressed your comment by removing the redundant 'completely'. I'll commit this in a few minutes.
Feb 15 2020
This seems like a straightforward improvement to me (assuming all the tests still pass of course). Thanks for cleaning this up!
Feb 14 2020
I found a test that tested the legacy PM opt -coro-split, but not the new PM opt -passes=coro-split. Now it tests both.
Feb 13 2020
Clean up coroutine intrinsics as part of the ThinLTO pre-link pipeline.
Rebase past D69930 so the patch applies cleanly to trunk.
Thanks, @jdoerfert! As per your suggestions I updated the docblock and the test comments, and I added a test specifically for LazyCallGraph. Built and tested everything with ASAN and it passes.
Feb 9 2020
Feb 3 2020
Update based on @jdoerfert's latest version of revision D70927. Previous versions added a public member function to CallGraphUpdater, the latest versions instead add a public member function to LazyCallGraph. This update does the same, it moves the new member function, from CallGraphUpdater::registerReferredToOutlinedFunction, to LazyCallGraph::addNewFunctionIntoRefSCC.
I mentioned in my last response that you moved the "problematic" logic into LazyCallGraph in your patch (D72226), or did you do something different? Please clarify what you want me to do that is different, or the same, to your patch. I'm happy to move logic into the LazyCallGraph class for example.
Sorry if it's presumptuous of me to request changes, but I guess @hfinkel may agree with me here that registerOutlinedFunction ought to be moved to LazyCallGraph or one of its inner classes.
Feb 1 2020
No, my only motivation was reading the interface and doing a double-take. I thought maybe the int return type was meant to not only signify whether the pointer was to the given type, but also the index at which that type existed in the union. I had to read the body of the function to learn that this wasn't the case. A bool return type, at least in my case, would have prevented that initial confusion.