[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. Whe

[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 clang/test/OpenMP/omp_with_loop

[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 clang/test/OpenMP/omp_wi

[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 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 q

[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 actual

[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 CoroSpl

[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 > (https://en.cppreference.com/w/cpp

[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 values

[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: > 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 som

[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 `Coro

[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 symm

[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. > 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. > 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 prob

[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 th

[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 (Su

[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 ne

[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: > > > Chua

[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. Corou

[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 that

[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 fi

[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 fronten

[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 f

[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 clang/li

[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/ https://revie

[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. 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 llvm-c

[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 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: clang/lib/CodeG

[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 SI

[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] 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 o

[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 th

[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: clang/lib/CodeGen/CGCoroutine

[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 clang/test/CodeGenCoroutines/c

[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: en

[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 llvm/include/llvm/IR/Intrins

[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 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 hide

[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 w

[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 LA

[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 before

[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 funct

[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] 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] 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 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 mislea

[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 corout

[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 cfe-

<    1   2