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

lxfind accepted this revision. lxfind added a comment. I also agree that we should try to keep the compiler simple and not support the complicated case. It should be fairly straightforward for a codebase to update fully to use std instead of std::experimental (we have a large coroutine codebase

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

lxfind accepted this revision. lxfind added a comment. This revision is now accepted and ready to land. LGTM. Thanks! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D108696/new/ https://reviews.llvm.org/D108696 ___ cfe-commits mailing list

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

lxfind added a comment. 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 migration? I assume that this is going to be a breaking change that existing code using

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

lxfind 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] D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context.

lxfind added a comment. Herald added subscribers: manas, steakhal, ASDenysPetrov, martong, dkrupp, donat.nagy, Szelethus, mikhail.ramalho, baloghadamsoftware. Hi! I have a question regarding the implementation of "VisitMaterializeTemporaryExpr". Specifically, I wonder if we should skip

[PATCH] D107155: [clang][deps] Substitute clang-scan-deps executable in lit tests

lxfind accepted this revision. lxfind added a comment. This revision is now accepted and ready to land. Thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107155/new/ https://reviews.llvm.org/D107155

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG822b92aae439: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after… (authored by lxfind). Repository: rG LLVM Github

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

lxfind added inline comments. Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2001 -if ((Shape.ABI == coro::ABI::Async || Shape.ABI == coro::ABI::Retcon || - Shape.ABI == coro::ABI::RetconOnce) && -!Shape.CoroSuspends.empty()) { - // Run the

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

lxfind updated this revision to Diff 355607. lxfind added a comment. fix warning Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95807/new/ https://reviews.llvm.org/D95807 Files: clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp

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

lxfind 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 << "\n");

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

lxfind updated this revision to Diff 355431. lxfind added a comment. Put the post-split ramp function back to the CGSCC worklist Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D95807/new/ https://reviews.llvm.org/D95807 Files:

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

lxfind added a comment. > 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 be inlined, but not when you run Inliner on the inlinee. Let's say coroutine A calls

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

lxfind added a comment. In D95807#2849053 , @aeubanks wrote: > this will run the function simplification pipeline twice on every single > function when coroutines are enabled, I don't think that's the intention > > I thought the intention was to do all

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

lxfind added a comment. In D95807#2846358 , @ChuanqiXu wrote: >> 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

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

lxfind updated this revision to Diff 355110. lxfind added a comment. Herald added subscribers: cfe-commits, qcolombet. Herald added a project: clang. After removing the legacy test command, I was finally able to update this patch. It's now ready for review. I will update the decription to

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG31eb696fc4cd: [Coroutines] Remove CoroElide from O0 pipeline (authored by lxfind). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105066/new/

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

lxfind added a comment. > Yeah, but it may be inlined after splitting, which could trigger coro elide. In O0, there is no inliner pass (after CoroSplit), so inlining should never happen. >> in fact, we should make it illegal to mark a coroutine "always_inline", >> because there is no

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

lxfind added a comment. In D105066#2845958 , @ChuanqiXu wrote: > 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

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

lxfind updated this revision to Diff 355073. lxfind added a comment. update tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D105066/new/ https://reviews.llvm.org/D105066 Files: clang/test/CodeGenCoroutines/coro-newpm-pipeline.cpp

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

lxfind created this revision. lxfind added reviewers: ChuanqiXu, rjmccall. Herald added subscribers: hoy, modimo, wenlei, hiraditya. lxfind requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. CoroElide pass works only when a

[PATCH] D102465: [Coroutines] Mark every parameter

lxfind created this revision. Herald added subscribers: ChuanqiXu, hoy, modimo, wenlei, hiraditya. lxfind requested review of this revision. Herald added subscribers: llvm-commits, cfe-commits, jdoerfert. Herald added projects: clang, LLVM. Repository: rG LLVM Github Monorepo

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

lxfind added a comment. > Sorry for the confusion. I think either overaligned or under-aligned could be > used here to describe the problem: either "Handle overaligned frame" or "Fix > under-aligned frame". Since c++ spec defines the former but not the later >

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

lxfind added a comment. Thanks for working on this. I am still having a bit hard time understanding the solution. A few questions: 1. I assume this patch is to solve the problem where the promise object is not aligned according to its alignof annotation, right? The title/wording is a bit

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

lxfind 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() ChuanqiXu wrote: > ychen wrote: > > ChuanqiXu wrote: > > > ychen wrote:

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

lxfind 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() Do we need to change __builtin_coro_size? The argument will always be 1,

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

lxfind planned changes to this revision. lxfind added a comment. Plan to add documentation, fix Legacy pass and address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100415/new/ https://reviews.llvm.org/D100415

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

lxfind added a comment. In D100282#2699171 , @rjmccall wrote: > MLIR is an in-tree project that can be updated. Sure, but I think there are some important differences. As far as I understand, in MLIR, unlike in C++/Swift frontend where a coroutine

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

lxfind added a comment. This broke MLIR tests. It seems that MLIR tests depend on CoroEarly to be able to annotate coroutine function properly based on the intrinsics. Given that, I am now convinced we shouldn't set the attribute in the frontend. Instead we should simply move CoroEarly to

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG2b50f5a4343f: [Coroutines] Move CoroEarly pass to before AlwaysInliner (authored by lxfind). Repository: rG LLVM Github Monorepo CHANGES SINCE

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

lxfind added inline comments. Comment at: llvm/lib/Transforms/Coroutines/CoroSplit.cpp:2231 // coroutine. struct CoroSplitLegacy : public CallGraphSCCPass { static char ID; // Pass identification, replacement for typeid ChuanqiXu wrote: > I am not familiar

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

lxfind added a comment. @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 benchmark. We can work on that in the future. For this patch, it's probably not worth it to

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

lxfind added a comment. In D100282#2687532 , @ychen wrote: > I think the setting is in CoroEarly from the beginning is that it is an > implementation detail? Clients should only worry about coroutine shape. > Maybe we could set `noinline` in frontends

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

lxfind updated this revision to Diff 337290. lxfind added a comment. some cleanups Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100415/new/ https://reviews.llvm.org/D100415 Files: clang/lib/CodeGen/CGCoroutine.cpp

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

lxfind created this revision. Herald added subscribers: ChuanqiXu, hoy, modimo, wenlei, hiraditya. lxfind requested review of this revision. Herald added subscribers: llvm-commits, cfe-commits, jdoerfert. Herald added projects: clang, LLVM. A coroutine has the following structure in LLVM IR:

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

lxfind updated this revision to Diff 337259. lxfind added a comment. Update test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100282/new/ https://reviews.llvm.org/D100282 Files: clang/lib/CodeGen/CGCoroutine.cpp

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

lxfind updated this revision to Diff 336850. lxfind added a comment. Set the attributes in Clang instead of CoroEarly Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100282/new/ https://reviews.llvm.org/D100282 Files:

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

lxfind added a comment. > 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, since it's really an IR invariant that it's present > on all unlowered coroutines. By

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

lxfind added a comment. In D100282#2682251 , @rjmccall wrote: > Why does this pass even exist? We should just expect the frontend to set the > attribute. It's not like frontends don't have to otherwise know that they're > emitting a coroutine; a ton

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

lxfind created this revision. lxfind added reviewers: junparser, dongAxis1944, rjmccall, ChuanqiXu. Herald added subscribers: hoy, modimo, wenlei, hiraditya. lxfind requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. Presplit

[PATCH] D98135: [OpenMP][InstrProfiling] Fix a missing instr profiling counter

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf490a5969bd5: [OpenMP][InstrProfiling] Fix a missing instr profiling counter (authored by lxfind). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D98135: [OpenMP][InstrProfiling] Fix a missing instr profiling counter

lxfind updated this revision to Diff 333417. lxfind added a comment. This revision is now accepted and ready to land. check null on S Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98135/new/ https://reviews.llvm.org/D98135 Files:

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc7a39c833af1: [Coroutine][Clang] Force emit lifetime intrinsics for Coroutines (authored by lxfind). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D98135: [OpenMP][InstrProfiling] Fix a missing instr profiling counter

lxfind added a comment. In D98135#2650940 , @ABataev wrote: > In D98135#2650914 , @lxfind wrote: > >> @ABataev, wondering if you have a timeline on this? >> Missing counters from OMP functions sometimes cause

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

lxfind abandoned this revision. lxfind added a comment. Abandoning in favor of D99227 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98638/new/ https://reviews.llvm.org/D98638

[PATCH] D98135: [OpenMP][InstrProfiling] Fix a missing instr profiling counter

lxfind added a comment. @ABataev, wondering if you have a timeline on this? Missing counters from OMP functions sometimes cause llvm-cov to crash because of data inconsistency. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98135/new/

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

lxfind updated this revision to Diff 38. lxfind added a comment. Address comments, and fix all tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99227/new/ https://reviews.llvm.org/D99227 Files: clang/lib/CodeGen/CGCoroutine.cpp

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

lxfind added a comment. In D99227#2646819 , @rjmccall wrote: > Is it feasible to outline the initial segment that you don't want to be part > of the coroutine, and then have coroutine splitting force that outlined > function to be inlined into the ramp

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

lxfind added inline comments. Comment at: clang/lib/CodeGen/CGDecl.cpp:1318 /// otherwise llvm::Value *CodeGenFunction::EmitLifetimeStart(uint64_t Size, llvm::Value *Addr) { ChuanqiXu wrote: > Can we sure

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

lxfind added a comment. 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 optimized even at O0? If so, I wonder if it > confuses programmers since they may

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

lxfind added a subscriber: lewissbaker. lxfind added a comment. > I think you just set `ShouldEmitLifetimeMarkers` correctly in the first place > instead of adding this as an extra condition to every place that considers > it, however. This was set when a CodeGenFunction is constructed, at

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

lxfind created this revision. Herald added subscribers: ChuanqiXu, hoy, modimo, wenlei. lxfind requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. tl;dr Correct implementation of Corouintes requires having lifetime intrinsics available.

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

lxfind added a comment. @bruno Thanks for the review! Comment at: clang/lib/CodeGen/CGCoroutine.cpp:221 CGF.EmitBlock(RealSuspendBlock); + } else if (ForcestackStart) { +Builder.CreateCall( bruno wrote: > ChuanqiXu wrote: > > lxfind wrote: > > >

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

lxfind added a comment. I am not sure how this would work, maybe I am missing something. But this patch tries to round up the frame pointer by looking at the difference between the alignment of new and the alignment of the frame. The alignment of new only gives you the guaranteed alignment for

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

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

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

lxfind added a comment. > Can't we did as inline comments? No, because it would have already been too late. SuspendExpr returns the result of __builtin_coro_resume(awaiter.await_suspend().address()), which is different from the result of awaiter.await_suspend(). We need to be able to control

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

lxfind added a comment. > Then if we want to put the result of the await_suspend in the stack, I think > we can do it under CodeGen part only. It should be easy to judge the return > type of await_suspend and create a call to llvm.coro.forcestack to the return > value of await_suspend. We

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

lxfind added a comment. > Then we need to answer the question: how can we **prove** that the result of > symmetric transfer and %gro are the **only** exceptions from the above rules. > Or how can we know the list of exceptions wouldn't get longer and longer in > the future? > > Then go back to

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

lxfind added a comment. Well, I guess another potential solution is to force emitting lifetime intrinsics for this part of coroutine in the front-end. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98638/new/ https://reviews.llvm.org/D98638

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

lxfind added a comment. > Here what I want to say is we **shouldn't** handle all the symmetric > transfer from the above analysis. And we shouldn't change the ASTNodes and > Sema part. We need to solve about the above pattern. It is not easy to give a > solution since user could implement

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

lxfind added inline comments. Comment at: clang/include/clang/AST/ExprCXX.h:4695 +/// afterwards on the stack. class CoroutineSuspendExpr : public Expr { friend class ASTStmtReader; lxfind wrote: > ChuanqiXu wrote: > > It looks strange for the change of

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

lxfind added a comment. 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 the fields of coroutine frame after the frame gets > destroyed). Or it just wastes

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

lxfind added inline comments. Comment at: clang/include/clang/AST/ExprCXX.h:4695 +/// afterwards on the stack. class CoroutineSuspendExpr : public Expr { friend class ASTStmtReader; ChuanqiXu wrote: > It looks strange for the change of `CoroutineSuspendExpr`

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

lxfind added a comment. In D98638#2628082 , @ChuanqiXu wrote: > 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

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

lxfind created this revision. Herald added subscribers: ChuanqiXu, hoy, modimo, wenlei, hiraditya. lxfind requested review of this revision. Herald added subscribers: llvm-commits, cfe-commits, jdoerfert. Herald added projects: clang, LLVM. One of the challenges with the alloca analysis in

[PATCH] D98135: [OpenMP][InstrProfiling] Fix a missing instr profiling counter

lxfind added a comment. In D98135#2617448 , @ABataev wrote: > In D98135#2617446 , @lxfind wrote: > >> In D98135#2617432 , @ABataev wrote: >> >>> There is a problem. We

[PATCH] D98135: [OpenMP][InstrProfiling] Fix a missing instr profiling counter

lxfind added a comment. In D98135#2617432 , @ABataev wrote: > There is a problem. We actually do not emit `S` here directly, instead, we > use `CodeGen` lambdas, which may not be equal to `S`, in some cases `S` is > `nullptr` here. It may result in not

[PATCH] D98135: [OpenMP][InstrProfiling] Fix a missing instr profiling counter

lxfind added a comment. Thanks. I am landing it. But feel free to comment here if anything isn't right. @ABataev Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98135/new/ https://reviews.llvm.org/D98135

[PATCH] D98135: [OpenMP][InstrProfiling] Fix a missing instr profiling counter

lxfind updated this revision to Diff 329517. lxfind added a comment. address comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98135/new/ https://reviews.llvm.org/D98135 Files: clang/lib/CodeGen/CGOpenMPRuntime.cpp

[PATCH] D98135: [OpenMP][InstrProfiling] Fix a missing instr profiling counter

lxfind updated this revision to Diff 328839. lxfind added a comment. add test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98135/new/ https://reviews.llvm.org/D98135 Files: clang/lib/CodeGen/CGOpenMPRuntime.cpp

[PATCH] D98135: [OpenMP][InstrProfiling] Fix a missing instr profiling counter

lxfind created this revision. lxfind added reviewers: ABataev, MaskRay. Herald added subscribers: hoy, modimo, wenlei, guansong, yaxunl. lxfind requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clang.

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

lxfind added a comment. Could you describe in more detail what problem this patch solves? Also, since you are adding a new intrinsics, please also update the coroutine documentation regarding this new intrinsics. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D92662: [Clang][Coroutine] Drop const attribute on pthread_self when coroutine is enabled

lxfind added a comment. In D92662#2446777 , @jyknight wrote: > I don't think we should change the meaning of `__attribute__((const))` to > exclude depending on thread-id. > > However, if we do want to do so, and call the existing uses of >

[PATCH] D92662: [Clang][Coroutine] Drop const attribute on pthread_self when coroutine is enabled

lxfind added a comment. In D92662#2443970 , @MaskRay wrote: > If the attribute is not suitable, glibc should drop it. The compiler can add > `readnone`/`readonly` if appropriate. It's a C library interface though, and Coroutine is likely too new for

[PATCH] D92662: [Clang][Coroutine] Drop const attribute on pthread_self when coroutine is enabled

lxfind updated this revision to Diff 310599. lxfind added a comment. Fix test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92662/new/ https://reviews.llvm.org/D92662 Files: clang/lib/Sema/SemaDecl.cpp

[PATCH] D92661: [RFC] Fix TLS and Coroutine

lxfind updated this revision to Diff 310575. lxfind added a comment. Herald added subscribers: nikic, kerbowa, jvesely. Fix all failing tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D92661/new/ https://reviews.llvm.org/D92661 Files:

[PATCH] D92661: [RFC] Fix TLS and Coroutine

lxfind added inline comments. Comment at: llvm/include/llvm/IR/Intrinsics.td:1309 +// Intrinsic to obtain the address of a thread_local variable. +def int_threadlocal : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty]>; + hoy wrote: > lxfind wrote: > > hoy wrote: > > >

[PATCH] D92661: [RFC] Fix TLS and Coroutine

lxfind added inline comments. Comment at: llvm/include/llvm/IR/Intrinsics.td:1309 +// Intrinsic to obtain the address of a thread_local variable. +def int_threadlocal : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty]>; + hoy wrote: > lxfind wrote: > > hoy wrote: > > >

[PATCH] D92661: [RFC] Fix TLS and Coroutine

lxfind added inline comments. Comment at: llvm/include/llvm/IR/Intrinsics.td:1309 +// Intrinsic to obtain the address of a thread_local variable. +def int_threadlocal : Intrinsic<[llvm_ptr_ty], [llvm_ptr_ty]>; + hoy wrote: > hoy wrote: > > With the intrinsic,

[PATCH] D92662: [Clang][Coroutine] Drop const attribute on pthread_self when coroutine is enabled

lxfind created this revision. Herald added subscribers: hoy, modimo, wenlei, modocache. lxfind requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This patch is to address https://bugs.llvm.org/show_bug.cgi?id=47833 A relevant discussion can

[PATCH] D92661: [RFC] Fix TLS and Coroutine

lxfind created this revision. Herald added subscribers: hoy, modimo, wenlei, steven_wu, modocache, hiraditya, mgorny. lxfind requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: llvm-commits, cfe-commits, sstefan1, jdoerfert. Herald added projects:

[PATCH] D86853: [modules] Fix crash in call to `FunctionDecl::setPure()`

lxfind added a comment. @rsmith, @v.g.vassilev hey I stamped this patch assuming it looks ok. But definitely shout at me if more feedback needs to be addressed. Happy to follow up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D86853/new/

[PATCH] D86853: [modules] Fix crash in call to `FunctionDecl::setPure()`

This revision was automatically updated to reflect the committed changes. Closed by commit rGc6c8d4a13ebd: [modules] Fix crash in call to `FunctionDecl::setPure()` (authored by andrewjcg, committed by lxfind). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D90990: [Coroutine][Sema] Cleanup temporaries as early as possible

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG19f077092343: [Coroutine][Sema] Cleanup temporaries as early as possible (authored by lxfind). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D90990: [Coroutine][Sema] Cleanup temporaries as early as possible

lxfind updated this revision to Diff 304233. lxfind added a comment. Add AST test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D90990/new/ https://reviews.llvm.org/D90990 Files: clang/lib/Sema/SemaCoroutine.cpp

[PATCH] D90990: [Coroutine][Sema] Cleanup temporaries as early as possible

lxfind added inline comments. Comment at: clang/lib/Sema/SemaCoroutine.cpp:475 + if (!AwaitSuspend) +return Calls; if (!AwaitSuspend->getType()->isDependentType()) { lxfind wrote: > bruno wrote: > > In case `AwaitSuspend` is null, is there any need to

[PATCH] D90990: [Coroutine][Sema] Cleanup temporaries as early as possible

lxfind added inline comments. Comment at: clang/lib/Sema/SemaCoroutine.cpp:475 + if (!AwaitSuspend) +return Calls; if (!AwaitSuspend->getType()->isDependentType()) { bruno wrote: > In case `AwaitSuspend` is null, is there any need to set

[PATCH] D90990: [Coroutine][Sema] Cleanup temporaries as early as possible

lxfind created this revision. Herald added subscribers: cfe-commits, modimo, wenlei, modocache. Herald added a project: clang. lxfind requested review of this revision. The original bug was discovered in T75057860. Clang front-end emits an AST that looks like this for an co_await expression: |

[PATCH] D89269: [Coroutine] Rename coro-semmetric-transfer.cpp and fix test failure

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd80ecdf27faf: [Coroutine] Rename coro-semmetric-transfer.cpp and possibly fix test failure (authored by lxfind). Repository: rG LLVM Github

[PATCH] D89066: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter

lxfind added a comment. Test failures are being fixed in https://reviews.llvm.org/D89269. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89066/new/ https://reviews.llvm.org/D89066 ___ cfe-commits mailing

[PATCH] D89269: [Coroutine] Rename coro-semmetric-transfer.cpp and possibly fix test failure

lxfind created this revision. lxfind added reviewers: wenlei, junparser. Herald added subscribers: cfe-commits, modimo, modocache. Herald added a project: clang. lxfind requested review of this revision. Some tests start to fail after https://reviews.llvm.org/D89066. It's because the size of

[PATCH] D89066: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter

lxfind added a comment. There seems to be build failures in the buildbot, but I don't understand why it's happening.. (unable to repro locally and the patterns seem reasonable) http://lab.llvm.org:8011/#/builders/12/builds/92/steps/7/logs/FAIL__Clang__coro-semmetric-transfer_cpp Repository:

[PATCH] D89066: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGdce8f2bb25ea: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter (authored by lxfind). Repository: rG LLVM Github

[PATCH] D89066: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter

lxfind updated this revision to Diff 297656. lxfind added a comment. Add more comments and TODO Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89066/new/ https://reviews.llvm.org/D89066 Files: clang/lib/Sema/SemaCoroutine.cpp

[PATCH] D89066: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter

lxfind added a comment. In D89066#2324291 , @junparser wrote: > In D89066#2324151 , @lxfind wrote: > >> In D89066#2324115 , @junparser >> wrote: >> >>> why we should not do

[PATCH] D89066: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter

lxfind added a comment. In D89066#2324115 , @junparser wrote: > why we should not do this with normal await call? To be honest, I don't know yet. My understanding of how expression cleanup and temp lifetime management is insufficient at the moment. But

[PATCH] D89066: [Coroutine][Sema] Only tighten the suspend call temp lifetime for final awaiter

lxfind created this revision. lxfind added reviewers: lewissbaker, wenlei, bruno, junparser, rjmccall. Herald added subscribers: cfe-commits, modimo, dexonsmith, modocache. Herald added a project: clang. lxfind requested review of this revision. In https://reviews.llvm.org/D87470 I added the

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

lxfind added a comment. In D87470#2273310 , @junparser wrote: > @lxfind , could you backport this to branch 11? I am actually seeing some problems with this change. Still investigating. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGdf477db5f9e0: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle (authored by lxfind). Repository: rG LLVM Github

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

lxfind added a comment. In D87470#2268911 , @rjmccall wrote: > Thanks, LGTM. Thank you for reviewing and the suggestions on testcase! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87470/new/

[PATCH] D87470: [Coroutine][Sema] Tighten the lifetime of symmetric transfer returned handle

lxfind updated this revision to Diff 291324. lxfind added a comment. remove asan option, not needed Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87470/new/ https://reviews.llvm.org/D87470 Files: clang/lib/Sema/SemaCoroutine.cpp

  1   2   >