[PATCH] D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment

This revision was automatically updated to reflect the committed changes. Closed by commit rG8b48d2437320: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid… (authored by ChuanqiXu). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment

ChuanqiXu added a comment. In D91245#2454504 , @MyDeveloperDay wrote: > Aside: Would you be prepared to take a look at D34225: [clang-format] Teach > clang-format how to handle C++ coroutines > which is pretty much

[PATCH] D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment

ChuanqiXu added a comment. In D91245#2454610 , @MyDeveloperDay wrote: > I don't like seeing users having to use ```// clang-format off``` > > auto try_sequence = [](int& ref) -> return_ignore { > /* clang-format off */ > for

[PATCH] D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment

ChuanqiXu updated this revision to Diff 311789. ChuanqiXu added a comment. add `verifyFormat("return *a", PASLeftStyle);` to clarify the change CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91245/new/ https://reviews.llvm.org/D91245 Files: clang/lib/Format/TokenAnnotator.cpp

[PATCH] D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment

ChuanqiXu added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:7759-7761 + // The default setting for PointerAlignment is PAS_Right. + // But if we set PointerAlignment as PAS_Left, the formatter + // would mis-format the pointer alignment.

[PATCH] D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment

ChuanqiXu updated this revision to Diff 311849. ChuanqiXu added a comment. Edit the comment to avoid misleading. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91245/new/ https://reviews.llvm.org/D91245 Files: clang/lib/Format/TokenAnnotator.cpp

[PATCH] D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment

ChuanqiXu added inline comments. Comment at: clang/unittests/Format/FormatTest.cpp:7759-7761 + // The default setting for PointerAlignment is PAS_Right. + // But if we set PointerAlignment as PAS_Left, the formatter + // would mis-format the pointer alignment.

[PATCH] D91243: [NFC][Coroutines] Remove unused `IsImplicit` argument in maybeTailCall

This revision was automatically updated to reflect the committed changes. Closed by commit rGcd89c4dbdd3a: [NFC][coroutines] remove unused argument in SemaCoroutine (authored by ChuanqiXu). Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github

[PATCH] D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment

ChuanqiXu added a comment. In D91245#2437518 , @MyDeveloperDay wrote: > What does `the behavior goes wrong` mean? why can't it be left aligned? Because when we use PointerAlignment attribute, I think we mean that the pointer need to be aligned in

[PATCH] D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment

ChuanqiXu added a comment. In D91245#2437741 , @Quuxplusone wrote: > Peanut gallery says: It seems to me that your root problem here is that > `co_yield` is not being recognized by the parser as a keyword, and so you're > seeing e.g. `co_yield* p;` the

[PATCH] D91245: [clang-format] Recognize c++ coroutine keywords as unary operator to avoid misleading pointer alignment

ChuanqiXu updated this revision to Diff 311485. ChuanqiXu added a comment. Re-upload patch with context CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91245/new/ https://reviews.llvm.org/D91245 Files: clang/lib/Format/TokenAnnotator.cpp clang/unittests/Format/FormatTest.cpp

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. In D97915#2819871 , @ychen wrote: > - Merge D102145 by @ChuanqiXu's request. Thanks a lot. Could you add me as a reviewer? So that I can see this patch in my main page. Repository: rG

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. It looks not easy to review completely. After a rough review, the first suggest is to remove the contents that should be in 'D102147 ', it makes the complex problem more complex I think. I would try to do more detailed review and

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. A remained question. - what's the semantics if user specified their allocation/deallocation functions? Previously, we discussed for the ::operator new and ::operator delete. But what would we do for user specified allocation/deallocation functions? It looks like we

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. In D97915#2832446 , @ychen wrote: > In D97915#2829581 , @ChuanqiXu wrote: > >> A remained question. >> >> - what's the semantics if user specified their allocation/deallocation >>

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. It misses the part in llvm now. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:437-475 +void emitDynamicAlignedDealloc(CodeGenFunction , + llvm::BasicBlock *AlignedFreeBB, + llvm::CallInst

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added inline comments. Comment at: clang/include/clang/AST/StmtCXX.h:356-359 Expr *Allocate = nullptr; Expr *Deallocate = nullptr; +Expr *AlignedAllocate = nullptr; +Expr *AlignedDeallocate = nullptr; Can't we merge these?

[PATCH] D102147: [Clang][Coroutines] Implement P2014R0 Option 1 behind -fcoroutines-aligned-alloc

ChuanqiXu added a comment. In D102147#2747939 , @ychen wrote: > In D102147#2747611 , @ChuanqiXu > wrote: > >> Since D97915 would fix the problem that >> the variables in the

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. > which should be correct. It is implemented by > CodeGenFunction::EmitBuiltinAlignTo. I agree it is correct. I just want to say we should comment it to avoid confusing. Since the patch could handle the case if the frontend tries to search `::operator new(size_t,

[PATCH] D102147: [Clang][Coroutines] Implement P2014R0 Option 1 behind -fcoroutines-aligned-alloc

ChuanqiXu added a comment. Since D97915 would fix the problem that the variables in the frame may not be aligned, I think this option `fcoroutines-aligned-alloc` won't affect normal programmers other than language lawyers. Do you think so? Repository: rG

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added inline comments. Comment at: clang/include/clang/AST/StmtCXX.h:356-359 Expr *Allocate = nullptr; Expr *Deallocate = nullptr; +Expr *AlignedAllocate = nullptr; +Expr *AlignedDeallocate = nullptr; ychen wrote: > ChuanqiXu wrote: >

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added inline comments. Comment at: clang/include/clang/AST/StmtCXX.h:356-359 Expr *Allocate = nullptr; Expr *Deallocate = nullptr; +Expr *AlignedAllocate = nullptr; +Expr *AlignedDeallocate = nullptr; ychen wrote: > ChuanqiXu wrote: >

[PATCH] D102465: [Coroutines] Mark every parameter

ChuanqiXu added a comment. If it would take times to mark parameter passed by value only, I think we could proceed with this one first. We can try to optimize it later. After all, correctness is most important. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. In D97915#2862419 , @ychen wrote: >> It looks not hard to implement. And we don't need to refactor the CodeGen >> part a lot. In this way, I think the main effort to support `::operator >> new(size_t, align_t)` would be in the

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. In D97915#2863615 , @ychen wrote: > That's just my conclusion based on @rjmccall's suggestion > (https://reviews.llvm.org/D100739#2717582) and my following responses. I guess you got the conclusion from this: > 2d. Use the

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. In D97915#2835147 , @ychen wrote: > In D97915#2832667 , @ChuanqiXu wrote: > >> In D97915#2832446 , @ychen wrote: >> >>> In D97915#2829581

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

ChuanqiXu added a comment. In D95807#2849120 , @lxfind wrote: > In D95807#2849053 , @aeubanks wrote: > >> this will run the function simplification pipeline twice on every single >> function when coroutines are

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. In D95807#2847449 , @lxfind wrote: > In D95807#2846358 , @ChuanqiXu wrote: > >>> note that we don't

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

ChuanqiXu added a comment. In D95807#2849128 , @lxfind wrote: >> If coroutine ramp function couldn't get inlined, it would disable coroutine >> elide optimization. Could you elaborate more on why do you want to do that? > > Ramp function will eventually

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

ChuanqiXu added inline comments. Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2112-2114 StringRef Value = Attr.getValueAsString(); LLVM_DEBUG(dbgs() << "CoroSplit: Processing coroutine '" << F.getName() << "' state: " << Value <<

[PATCH] D105066: [Coroutines] Remove CoroElide from O0 pipeline

ChuanqiXu added a comment. In D105066#2845978 , @lxfind wrote: > Coroutine functions cannot be inlined before splitting, even if it's marked > "always_inline" Yeah, but it may be inlined after splitting, which could trigger coro elide. > in fact, we

[PATCH] D105066: [Coroutines] Remove CoroElide from O0 pipeline

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. > That's a separate topic though. Let's agree on this diff first and then I can > explain more about the always_inline issue. Yeah, maybe we should discuss it on the llvm-dev thread.

[PATCH] D105066: [Coroutines] Remove CoroElide from O0 pipeline

ChuanqiXu added a comment. On O0, it is possible to inline if the user marked the function with `always_inline`. Since CoroElide is kind of optimization, it should be OK to skip in O0. Out of curiosity, what's the reason that you want to remove it? Repository: rG LLVM Github Monorepo

[PATCH] D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened

ChuanqiXu added a comment. > note that we don't really need to run Inliner again on the ramp function > after split This isn't accurate. The inline may run again for ramp function after split and it's required by coro elide. It seems like that we don't need the attribute `CORO_PRESPLIT_ATTR`

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. In D97915#2860916 , @ychen wrote: > In D97915#2859237 , @ChuanqiXu wrote: > >> In D97915#2848816 , @ychen wrote: >> Thanks for clarifying.

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. In D97915#2861036 , @ychen wrote: > In D97915#2860984 , @ChuanqiXu wrote: > >> In D97915#2860916 , @ychen wrote: >> >>> In D97915#2859237

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. In D97915#2848816 , @ychen wrote: >> Thanks for clarifying. Let's solve the semantics problem first. >> With the introduction about 'raw frame', I think it's necessary to introduce >> this concept in the section

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. I am a little busy this week. I would try to look into this next week if possible. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97915/new/ https://reviews.llvm.org/D97915

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

ChuanqiXu added a comment. In D100739#2713579 , @ychen wrote: > In D100739#2711698 , @ChuanqiXu > wrote: > >>> This is an alternative to D97915 which >>> missed proper

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

ChuanqiXu added a comment. In D100739#2718514 , @ychen wrote: > Oh, right now C++ coroutine standard is written in the way that the aligned > new is not searched by the frontend. The limitation will be lifted in C++23 > (hopefully). I see. I am

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

ChuanqiXu added a comment. In D100739#2717582 , @rjmccall wrote: > That's not really what I meant, no. I meant it would be better to find a way > to use an allocator that promises to return a well-aligned value when > possible. We've talked about this

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. May I ask a question may be too simple? What if the user specify the alignment for promise (or any other local variables) to 128 or even 256? Since it looks like all the discuss before assumes that the largest alignment requirement is 64. Repository: rG LLVM

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. In D97915#2727787 , @ychen wrote: > In D97915#2727759 , @ChuanqiXu wrote: > >> May I ask a question may be too simple? What if the user specify the >> alignment for promise (or any other

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

ChuanqiXu added a comment. In D100739#2727808 , @ychen wrote: > In D100739#2718528 , @ChuanqiXu > wrote: > >> In D100739#2718514 , @ychen wrote: >> >>> Oh, right now C++

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. This code snippets confused me before: coro.alloc.align: ; preds = %coro.check.align %mask = sub i64 %11, 1 %intptr = ptrtoint i8* %call to i64 %over_boundary = add i64 %intptr, %mask %inverted_mask = xor i64 %mask, -1

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

ChuanqiXu added a comment. In D99227#2646719 , @lxfind wrote: > In D99227#2646568 , @ChuanqiXu wrote: > >> Only one problem I had for emitting lifetime markers even at O0 is that >> would action make allocas to be

[PATCH] D99227: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines

ChuanqiXu added a comment. Only one problem I had for emitting lifetime markers even at O0 is that would action make allocas to be optimized even at O0? If so, I wonder if it confuses programmers since they may find some variables disappear surprisingly. Or there would be no optimization since

[PATCH] D99245: [Driver] Add -fno-split-stack

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG20b4f484d16f: [Driver] Add -fno-split-stack (authored by ChuanqiXu). Herald added a project: clang. Herald added a subscriber: cfe-commits. Changed

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

ChuanqiXu added a comment. It looks like there are two things this patch wants to do: 1. Don't put the temporary generated by symmetric-transfer on the coroutine frame. 2. Offer a mechanism to force some values (it is easy to extend Alloca to Value) to put in the stack instead of the coroutine

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

ChuanqiXu added a comment. In D98638#2630864 , @lxfind wrote: > That's a fair point. I agree that we have no guarantee these are the only two > cases. > It does seem to me that coroutine implementation somewhat relies on proper > lifetime markers so

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:221 CGF.EmitBlock(RealSuspendBlock); + } else if (ForcestackStart) { +Builder.CreateCall( lxfind wrote: > ChuanqiXu wrote: > > ChuanqiXu wrote: > > > can we rewrite it into:

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

ChuanqiXu added a comment. In D98638#2630778 , @lxfind wrote: > What do you think is the fundamental problem, though? It is hard to give a formal description for the problem. Let me try to explain it. What I want to say here is about rules that decide

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

ChuanqiXu added a comment. In D98638#2630607 , @lxfind wrote: > In D98638#2628082 , @ChuanqiXu wrote: > >> I am a little confused about the first problem. Would it cause the program >> to crash? (e.g., we access

[PATCH] D98638: [RFC][Coroutine] Force stack allocation after await_suspend() call

ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/CGCoroutine.cpp:221 CGF.EmitBlock(RealSuspendBlock); + } else if (ForcestackStart) { +Builder.CreateCall( ChuanqiXu wrote: > can we rewrite it into: > ``` > else if (SuspendRet != nullptr

[PATCH] D96316: [ASTMatcher] Add AST Matcher support for C++20 coroutine keywords

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG55486161fa0b: [ASTMatcher] Add AST Matcher support for C++20 coroutine keywords (authored by ChuanqiXu). Herald added a project: clang. Herald added

[PATCH] D97915: [Coroutines] Handle overaligned frame allocation

ChuanqiXu added a comment. I am a little confusing about the problem. The example in the link tells the align of the `promise` instead of the `frame`. The address of `promise` and `frame` is not same. It looks like you're trying to do: + +---+

[PATCH] D100282: [Coroutines] Move CoroEarly pass to before AlwaysInliner

ChuanqiXu added a comment. In D100282#2682297 , @lxfind wrote: >> Ah, if the pass does more than just setting the attribute, then sure, it >> makes sense to keep it. But I do think we should be requiring the attribute >> to be added by frontends,

[PATCH] D100282: [Coroutines] Move CoroEarly pass to before AlwaysInliner

ChuanqiXu added a comment. This patch looks OK to me. > The best we can do is lower the coroutine and then inline the ramp function, > which is not really the same thing. So warning about marking a coroutine > always_inline does make some sense. It makes sense to emit a warning instead of an

[PATCH] D100415: [Coroutines] Split coroutine during CoroEarly into an init and ramp function

ChuanqiXu added a comment. It looks like this code may trigger assertion in `CoroInstr.h` for `CoroIdInst::setCoroutineSelf`. Also, since this patch would enlarge the coroutine frame, it may affect the performance naturally. I **believe** it wouldn't really matter. I just find that we need

[PATCH] D100415: [Coroutines] Split coroutine during CoroEarly into an init and ramp function

ChuanqiXu added a comment. In D100415#2691666 , @lxfind wrote: > @ChuanqiXu Thank you for the detailed review! Really appreciate it. > I agree we should create a coroutine benchmark at some point, ideally some > realistic production-code driven

[PATCH] D100415: [Coroutines] Split coroutine during CoroEarly into an init and ramp function

ChuanqiXu added a comment. The overall idea looks good to me. Since this is a fundamental and large change, I need time to run it actually and look into details. Noticed that this patch introduces new intrinsics and new concept `init function`, it may be needed to add them in the coroutines

[PATCH] D100282: [Coroutines] Set presplit attribute in Clang instead of CoroEarly pass

ChuanqiXu accepted this revision. ChuanqiXu added a comment. I agree with the idea we should set the attribute in the frontend or move Coro-early pass in the front. It looks more weird to add a `noinline` attribute and move it later. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D99839: [C++, test] Fix typo in NSS* vars

ChuanqiXu added inline comments. Comment at: clang/test/CodeGenCXX/split-stacks.cpp:30 // CHECK-NOSEGSTK: define dso_local i32 @_Z7nosplitv() [[NSS1:#[0-9]+]] { // CHECK-NOSEGSTK: define linkonce_odr dso_local i32 @_Z8tnosplitIiEiv() [[NSS2:#[0-9]+]] comdat { +//

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

ChuanqiXu added a comment. In D100739#2700324 , @ychen wrote: > In D100739#2700273 , @ChuanqiXu > wrote: > >> I hadn't looked into the details. I would try to make it. >> But from my understanding to this

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

ChuanqiXu added a comment. I hadn't looked into the details. I would try to make it. But from my understanding to this problem, the correct solution should remain the previous behavior if the end user doesn't specify `alignas` for `promise_type`. I mean, it shouldn't make so many test cases

[PATCH] D99839: [C++, test] Fix typo in NSS* vars

ChuanqiXu added inline comments. Comment at: clang/test/CodeGenCXX/split-stacks.cpp:30 // CHECK-NOSEGSTK: define dso_local i32 @_Z7nosplitv() [[NSS1:#[0-9]+]] { // CHECK-NOSEGSTK: define linkonce_odr dso_local i32 @_Z8tnosplitIiEiv() [[NSS2:#[0-9]+]] comdat { +//

[PATCH] D99839: [C++, test] Fix typo in NSS* vars

ChuanqiXu added inline comments. Comment at: clang/test/CodeGenCXX/split-stacks.cpp:30 // CHECK-NOSEGSTK: define dso_local i32 @_Z7nosplitv() [[NSS1:#[0-9]+]] { // CHECK-NOSEGSTK: define linkonce_odr dso_local i32 @_Z8tnosplitIiEiv() [[NSS2:#[0-9]+]] comdat { +//

[PATCH] D99839: [C++, test] Fix typo in NSS* vars

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99839/new/ https://reviews.llvm.org/D99839

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

ChuanqiXu added inline comments. Comment at: clang/docs/LanguageExtensions.rst:2689 - size_t __builtin_coro_size() + size_t __builtin_coro_size(bool alloc) void *__builtin_coro_frame() ychen wrote: > ChuanqiXu wrote: > > ychen wrote: > > > lxfind wrote:

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

ChuanqiXu added a comment. There is something I am still confused about these two patch. Maybe I don't get the problem right. The example problem shows if user uses `alignas` to specify the alignment for promise_type, the actual alignment for the promise isn't correctly due to compiler didn't

[PATCH] D100739: [Coroutines] Handle overaligned frame allocation (2)

ChuanqiXu added a comment. > This is an alternative to D97915 which > missed proper deallocation of the over-allocated frame. This patch handles > both allocations and deallocations. If D97915 is not needed, we should abandon

[PATCH] D108615: [Coroutines] [libcxx] Move coroutine component out of experimental namespace

ChuanqiXu added a comment. In D108615#2962618 , @lxfind wrote: > I am not familiar with the process of when to move something out of > experimental, but I do wonder how this is normally done so that people who > uses coroutines can have a smooth

[PATCH] D108615: [Coroutines] [libcxx] Move coroutine component out of experimental namespace

ChuanqiXu abandoned this revision. ChuanqiXu added a comment. Oh, I find why the CI still fails. Since the compiler used to compile libcxx in the CI doesn't contain the change in this patch! So it would still lookup coroutine components in std::experimental namespace. So here are the failures.

[PATCH] D108905: [ItaniumCXXABI] Set nounwind on __cxa_begin_catch/__cxa_end_catch

ChuanqiXu accepted this revision. ChuanqiXu added a comment. This revision is now accepted and ready to land. In D108905#2971773 , @MaskRay wrote: > In D108905#2971761 , @ChuanqiXu > wrote: > >> Thanks for

[PATCH] D108905: [ItaniumCXXABI] Set nounwind on __cxa_begin_catch/__cxa_end_catch nounwind

ChuanqiXu added a comment. Thanks for looking into this! I am not familiar with exception handling. But from the comments, I worried that if it is a problem if there are exceptions whose destructor could throw (although it is not usual nor good). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D108905: [ItaniumCXXABI] Set nounwind on __cxa_begin_catch/__cxa_end_catch

ChuanqiXu added inline comments. Comment at: clang/lib/CodeGen/ItaniumCXXABI.cpp:4435-4448 struct CallEndCatch final : EHScopeStack::Cleanup { CallEndCatch(bool MightThrow) : MightThrow(MightThrow) {} bool MightThrow; void Emit(CodeGenFunction , Flags flags)

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

ChuanqiXu added a comment. @rjmccall @lxfind @junparser hi, do you feel comfortable with this? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/ https://reviews.llvm.org/D108696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

ChuanqiXu added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:11015 + "coroutine_traits defined in std::experimental namespace is deprecated. " + "Considering update libcxx and include instead of .">, + InGroup; avogelsgesang

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

ChuanqiXu added a comment. In D108696#2983021 , @ldionne wrote: > Based on the commit description, I don't understand this change at all. Why > do we want to tweak the name lookup just for `std::coroutine`? Yes, we do > have an action item to finish

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

ChuanqiXu planned changes to this revision. ChuanqiXu added a comment. We should proceed after D108697 accepted. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/ https://reviews.llvm.org/D108696

[PATCH] D108905: [ItaniumCXXABI] Make __cxa_end_catch calls unconditionally nounwind

ChuanqiXu added a comment. In D108905#2975806 , @rjmccall wrote: > In D108905#2975712 , @rsmith wrote: > >> No decision as yet, but so far it looks very likely that we'll settle on the >> rule that exceptions

[PATCH] D105877: [Coroutines] Run coroutine passes by default

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8a1727ba51d2: [Coroutines] Run coroutine passes by default (authored by ChuanqiXu). Herald added a project: clang. Herald added a subscriber:

[PATCH] D105877: [Coroutines] Run coroutine passes by default

ChuanqiXu added a comment. In D105877#2923257 , @nikic wrote: > I noticed that this change had a measurable impact on `O0` memory usage, > which I wouldn't have expected >

[PATCH] D105877: [Coroutines] Run coroutine passes by default

ChuanqiXu added a comment. In D105877#2924157 , @ChuanqiXu wrote: > In D105877#2923257 , @nikic wrote: > >> I noticed that this change had a measurable impact on `O0` memory usage, >> which I wouldn't have

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

ChuanqiXu added a comment. In D108696#3018045 , @ldionne wrote: > This sounds more reasonable to me, however we need to ship `` in > libc++ before we enable this, or else we're going to start suggesting that > users include `` when we don't have it.

[PATCH] D110215: [C++2a] [Module] Support extern C/C++ semantics

ChuanqiXu added a comment. @rsmith @aaron.ballman gentle ping~ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110215/new/ https://reviews.llvm.org/D110215 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D110215: [C++2a] [Module] Support extern C/C++ semantics

ChuanqiXu updated this revision to Diff 379907. ChuanqiXu added a comment. Herald added a subscriber: dexonsmith. Address the comments from @rsmith. It looks much better now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110215/new/ https://reviews.llvm.org/D110215 Files:

[PATCH] D110215: [C++2a] [Module] Support extern C/C++ semantics

ChuanqiXu updated this revision to Diff 378401. ChuanqiXu added a comment. Address the comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110215/new/ https://reviews.llvm.org/D110215 Files: clang/include/clang/Sema/Sema.h clang/lib/Sema/SemaDecl.cpp

[PATCH] D110215: [C++2a] [Module] Support extern C/C++ semantics

ChuanqiXu added inline comments. Comment at: clang/lib/Sema/SemaDecl.cpp:9351 + (NewFD->isExternCContext() || NewFD->isExternCXXContext())) { +if (!getGlobalModule()) + Diag(NewFD->getLocation(), aaron.ballman wrote: > I'm a bit

[PATCH] D110215: [C++2a] [Module] Support extern C/C++ semantics

ChuanqiXu updated this revision to Diff 378415. ChuanqiXu added a comment. Format. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110215/new/ https://reviews.llvm.org/D110215 Files: clang/include/clang/Sema/Sema.h clang/lib/Sema/SemaDecl.cpp clang/lib/Sema/SemaModule.cpp

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

ChuanqiXu added a comment. In D108696#3069723 , @avogelsgesang wrote: > Hi @ChuanqiXu > > sorry for what might be naive questions, but just to make sure I understand > the context of this patch correctly: Hi, you are welcome : ) > This patch fixes

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

ChuanqiXu added a comment. @ldionne @Quuxplusone gentle ping~ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/ https://reviews.llvm.org/D108696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

ChuanqiXu added a comment. @ldionne @Quuxplusone Now the compiler wouldn't emit warning for any more. So I think this is good. Do you feel good with this? This is necessary to conform the implementation of coroutine since the compiler would search coroutine components in corresponding

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

ChuanqiXu added a comment. @Quuxplusone gentle ping~ CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/ https://reviews.llvm.org/D108696 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

ChuanqiXu added a comment. In D108696#3083081 , @Quuxplusone wrote: > In D108696#3082866 , @ChuanqiXu > wrote: > >> @Quuxplusone gentle ping~ > > I think this PR is mostly above my pay grade. :) Sorry for

[PATCH] D108696: [Coroutines] [Frontend] Lookup in std namespace first

ChuanqiXu added inline comments. Comment at: clang/test/SemaCXX/coroutines-exp-namespace.cpp:2 +// This file is the same with coroutines.cpp except the coroutine components are defined in std::experimental namespace. +// This intention of this test is to make sure the legacy

[PATCH] D110215: [C++2a] [Module] Support extern C/C++ semantics

ChuanqiXu added a comment. @aaron.ballman @urnathan @rsmith ping. Might you take a look? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110215/new/ https://reviews.llvm.org/D110215 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D115790: [Coroutines] Set presplit attribute in Clang

ChuanqiXu added a comment. In D115790#3199905 , @ezhulenev wrote: > There are two places where in MLIR you can put an attribute to coroutine > functions: > > 1. >

[PATCH] D115790: [Coroutines] Set presplit attribute in Clang and mlir

ChuanqiXu updated this revision to Diff 396254. ChuanqiXu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115790/new/ https://reviews.llvm.org/D115790 Files: clang/lib/CodeGen/CGCoroutine.cpp clang/test/CodeGenCoroutines/coro-always-inline.cpp

[PATCH] D116351: Update Bug report URL to Github Issues

ChuanqiXu updated this revision to Diff 396737. ChuanqiXu marked an inline comment as done. ChuanqiXu added a comment. Drop the change in lld. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D116351/new/ https://reviews.llvm.org/D116351 Files: clang-tools-extra/docs/clang-doc.rst

  1   2   3   4   5   6   7   8   9   10   >