[WIP] Go-Compatible Coroutine MVP #1532
Conversation
Add coroutine-based goroutine implementation with: - Coroutine wrapper generation for go statements - LLVM coroutine intrinsics (coro.id, coro.begin, coro.suspend, etc.) - Runtime scheduler with cooperative multitasking - Coroutine state management and memory allocation - Support for both AMD64 and cross-platform builds via LLGO_CORO env Includes test cases, runtime support functions (CoroSpawn, CoroSched), and comprehensive unit tests for the new coroutine infrastructure.
Add coroSuspend and coroResume intrinsic functions to enable explicit coroutine suspension and resumption. Key changes include: - Add corosuspend test case with expected LLVM IR output - Implement llgo.coroSuspend and llgo.coroResume intrinsics in compiler - Update runtime coroutine queue to track suspended coroutines - Add CoroSuspend runtime function with proper LLVM suspend point - Enable coroutines to be explicitly suspended and later resumed via their handle pointer
Summary of ChangesHello @MeteorsLiu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a foundational MVP for integrating LLVM's coroutine capabilities into Go, aiming to provide an alternative, potentially more efficient, implementation for goroutines. It establishes a "dual-symbol" compilation strategy where functions can have both synchronous and suspendable coroutine versions. The changes span compiler modifications to generate and manage these coroutine functions, runtime support for scheduling them, and a novel mechanism for propagating the execution mode across function calls. This work lays the groundwork for transparently leveraging LLVM coroutines while maintaining Go's concurrency model. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-documented MVP for implementing Go-compatible coroutines using LLVM's coroutine infrastructure. The changes are extensive, touching the compiler, SSA representation, and runtime to create a dual-symbol approach where go statements are compiled to use suspendable $coro function variants. While the overall architecture is promising, my review has identified several critical issues that must be addressed. These include a lack of integration with the Go garbage collector for coroutine frames, which could lead to memory corruption, and a non-thread-safe scheduler implementation. Additionally, there are high-severity issues related to a hardcoded user path and unfriendly compiler panics, along with medium-severity concerns about code duplication and test correctness. These foundational issues should be resolved to ensure the stability and correctness of this new concurrency model.
| // coro.alloc block: allocate memory using C malloc | ||
| // (LLVM coroutine frames need explicit memory management, not GC) | ||
| b.SetBlock(allocBlk) | ||
| size := b.CoroSize() | ||
| // Convert i64 to uintptr for malloc compatibility | ||
| sizeAsUintptr := b.Convert(prog.Uintptr(), size) | ||
| mem := b.malloc(sizeAsUintptr) | ||
| b.Jump(beginBlk) |
There was a problem hiding this comment.
The coroutine frame is allocated using malloc, which is outside the Go garbage collector's purview. The comment correctly notes that these frames need explicit memory management. However, if any Go pointers (strings, slices, interface values, pointers to GC-managed objects) are spilled into this coroutine frame, the GC will not be able to trace them. This can lead to premature collection of live objects and subsequent memory corruption (use-after-free). This is a critical correctness issue.
The implementation needs a mechanism to inform the Go GC about the pointers within the coroutine frame, possibly by registering the frame as a root or by generating a stack map for it, similar to how goroutine stacks are handled. Your design document goroutine-vs-llvm-coroutine.md correctly identifies this as "问题四：GC 扫描" (Problem 4: GC Scanning), but the current implementation doesn't seem to address it.
| var ( | ||
| // runQueue holds coroutines that are ready to run | ||
| coroRunQueue coroQueue | ||
|
|
||
| // currentCoro is the currently executing coroutine (nil if in main thread) | ||
| currentCoro CoroHandle | ||
| ) |
There was a problem hiding this comment.
The global coroutine run queue coroRunQueue and currentCoro are not protected by any synchronization mechanism (like a mutex). If the scheduler were to run on multiple OS threads, this would lead to data races when accessing the queue. While the current MVP might be single-threaded (running CoroSchedule after main), this design is fundamentally not thread-safe and will cause severe issues if the model is extended to support multi-threaded execution, which is a standard expectation for Go's concurrency. This should be addressed by adding proper locking to all accesses of the shared scheduler state.
| "Bash(go list:*)", | ||
| "Bash(go build:*)", | ||
| "WebSearch", | ||
| "Bash(LLGO_ROOT=/Users/haolan/project/t1/llgo LLGO_CORO=1 go run:*)" |
There was a problem hiding this comment.
The path /Users/haolan/project/t1/llgo is hardcoded. This path is specific to a single user's machine and will cause builds to fail for other developers or in CI/CD environments. This should be replaced with a relative path or an environment variable to ensure portability.
| "Bash(LLGO_ROOT=/Users/haolan/project/t1/llgo LLGO_CORO=1 go run:*)" | |
| "Bash(LLGO_ROOT=${LLGO_PROJECT_ROOT:-/path/to/default} LLGO_CORO=1 go run:*)" |
| if fnName == "" || IsCoroName(fnName) { | ||
| panic("goCoro: closures and method values not supported in LLVM coroutine mode, use named functions") | ||
| } |
There was a problem hiding this comment.
The goCoro function panics when encountering a closure or method value in a go statement. While it's acceptable for an MVP to have limitations, a compiler panic provides a poor user experience. It would be better to detect this situation and emit a clear, user-friendly compile-time error instead of panicking. This would guide the user on how to fix their code to work with the current limitations.
| // compileCoroFuncDecl generates the $coro version of a function for LLVM coroutine mode. | ||
| // The coro version has the same logic but: | ||
| // - Uses presplitcoroutine attribute | ||
| // - Has coro prologue (coro.id, coro.begin) | ||
| // - Has coro epilogue (coro.free, coro.end) | ||
| // - Returns ptr (coroutine handle) instead of original return type | ||
| func (p *context) compileCoroFuncDecl(pkg llssa.Package, f *ssa.Function, origFn llssa.Function, name string, sig *types.Signature, hasCtx bool, state pkgState, dbgEnabled, dbgSymsEnabled bool) { | ||
| coroName := llssa.CoroName(name) | ||
|
|
||
| // Create coro function using NewCoroFunc (adds presplitcoroutine attribute) | ||
| coroFnWrapper := pkg.NewCoroFunc(name, sig, llssa.InGo) | ||
| coroFn := coroFnWrapper.Fn | ||
|
|
||
| // Store in coroFuncs map for call redirection | ||
| p.coroFuncs[name] = coroFn | ||
|
|
||
| nblk := len(f.Blocks) | ||
| // Create nblk + 1 blocks: block 0 for coro entry, blocks 1..nblk for original body | ||
| coroFn.MakeBlocks(nblk + 1) | ||
| if f.Recover != nil { | ||
| // Recover block index is shifted by 1 in coro version | ||
| coroFn.SetRecover(coroFn.Block(f.Recover.Index + 1)) | ||
| } | ||
|
|
||
| // Add compilation closure for coro version | ||
| p.inits = append(p.inits, func() { | ||
| p.fn = coroFn | ||
| p.state = state | ||
| p.inCoroFunc = true // Mark that we're compiling coro version | ||
| defer func() { | ||
| p.fn = nil | ||
| p.inCoroFunc = false | ||
| p.coroState = nil | ||
| }() | ||
| p.phis = nil | ||
| if dbgSymsEnabled { | ||
| p.paramDIVars = make(map[*types.Var]llssa.DIVar) | ||
| } else { | ||
| p.paramDIVars = nil | ||
| } | ||
| if debugInstr { | ||
| log.Println("==> CoroFuncBody", coroName) | ||
| } | ||
| b := coroFn.NewBuilder() | ||
|
|
||
| // Block 0 is coro entry, body starts at block 1 | ||
| bodyStartBlk := coroFn.Block(1) | ||
|
|
||
| // Set builder to block 0 (coro entry) and generate prologue | ||
| b.SetBlock(coroFn.Block(0)) | ||
| p.coroState = b.CoroFuncPrologue(bodyStartBlk) | ||
|
|
||
| p.bvals = make(map[ssa.Value]llssa.Expr) | ||
| off := make([]int, len(f.Blocks)) | ||
| for i, block := range f.Blocks { | ||
| off[i] = p.compileCoroPhis(b, block) | ||
| } | ||
|
|
||
| p.blkInfos = blocks.Infos(f.Blocks) | ||
| i := 0 | ||
| for { | ||
| block := f.Blocks[i] | ||
| p.compileCoroBlock(b, block, off[i]) | ||
| if i = p.blkInfos[i].Next; i < 0 { | ||
| break | ||
| } | ||
| } | ||
| for _, phi := range p.phis { | ||
| phi() | ||
| } | ||
|
|
||
| // Generate coro epilogue | ||
| b.CoroFuncEpilogue(p.coroState) | ||
|
|
||
| b.EndBuild() | ||
| }) | ||
| } | ||
|
|
||
| // compileCoroPhis compiles phi nodes for a block in coro version. | ||
| // Block indices are shifted by 1 (block 0 is coro entry). | ||
| func (p *context) compileCoroPhis(b llssa.Builder, block *ssa.BasicBlock) int { | ||
| fn := p.fn | ||
| // In coro version, original block index N maps to block N+1 | ||
| blk := fn.Block(block.Index + 1) | ||
| b.SetBlockEx(blk, llssa.AtEnd, false) | ||
| if ninstr := len(block.Instrs); ninstr > 0 { | ||
| if isPhi(block.Instrs[0]) { | ||
| n := 1 | ||
| for n < ninstr && isPhi(block.Instrs[n]) { | ||
| n++ | ||
| } | ||
| rets := make([]llssa.Expr, n) | ||
| for i := 0; i < n; i++ { | ||
| iv := block.Instrs[i].(*ssa.Phi) | ||
| rets[i] = p.compileCoroPhi(b, iv) | ||
| } | ||
| for i := 0; i < n; i++ { | ||
| iv := block.Instrs[i].(*ssa.Phi) | ||
| p.bvals[iv] = rets[i] | ||
| } | ||
| return n | ||
| } | ||
| } | ||
| return 0 | ||
| } | ||
|
|
||
| // compileCoroPhi compiles a phi node in coro version. | ||
| // Block indices are shifted by 1. | ||
| func (p *context) compileCoroPhi(b llssa.Builder, v *ssa.Phi) (ret llssa.Expr) { | ||
| phi := b.Phi(p.type_(v.Type(), llssa.InGo)) | ||
| ret = phi.Expr | ||
| p.phis = append(p.phis, func() { | ||
| preds := v.Block().Preds | ||
| bblks := make([]llssa.BasicBlock, len(preds)) | ||
| for i, pred := range preds { | ||
| // In coro version, original block index N maps to block N+1 | ||
| bblks[i] = p.fn.Block(pred.Index + 1) | ||
| } | ||
| edges := v.Edges | ||
| phi.AddIncoming(b, bblks, func(i int, blk llssa.BasicBlock) llssa.Expr { | ||
| b.SetBlockEx(blk, llssa.BeforeLast, false) | ||
| return p.compileValue(b, edges[i]) | ||
| }) | ||
| }) | ||
| return | ||
| } | ||
|
|
||
| // compileCoroBlock compiles a block in coro version. | ||
| // Block indices are shifted by 1, and Return statements are transformed. | ||
| func (p *context) compileCoroBlock(b llssa.Builder, block *ssa.BasicBlock, n int) llssa.BasicBlock { | ||
| var fn = p.fn | ||
| var instrs = block.Instrs[n:] | ||
| // In coro version, original block index N maps to block N+1 | ||
| var ret = fn.Block(block.Index + 1) | ||
| b.SetBlock(ret) | ||
|
|
||
| last := len(instrs) - 1 | ||
| for i, instr := range instrs { | ||
| if debugInstr { | ||
| log.Printf("==> compileCoroInstr %T: %v\n", instr, instr) | ||
| } | ||
|
|
||
| // Skip terminators - we handle them separately with adjusted block indices | ||
| if i == last { | ||
| switch instr.(type) { | ||
| case *ssa.Return, *ssa.Jump, *ssa.If, *ssa.Panic: | ||
| continue // Don't compile, handle below | ||
| } | ||
| } | ||
|
|
||
| // For other instructions, compile normally | ||
| p.compileInstr(b, instr) | ||
| } | ||
|
|
||
| // Handle flow control with adjusted block indices | ||
| if last >= 0 { | ||
| switch term := instrs[last].(type) { | ||
| case *ssa.Return: | ||
| // In coro version, return should do final suspend and return handle | ||
| p.compileCoroReturn(b, term) | ||
| case *ssa.Jump: | ||
| // Jump to target block (index + 1 in coro version) | ||
| // term.Block().Succs[0] is the target block of the jump | ||
| targetBlk := fn.Block(term.Block().Succs[0].Index + 1) | ||
| b.Jump(targetBlk) | ||
| case *ssa.If: | ||
| // Branch to then/else blocks (index + 1 in coro version) | ||
| cond := p.compileValue(b, term.Cond) | ||
| succs := term.Block().Succs | ||
| thenBlk := fn.Block(succs[0].Index + 1) | ||
| elseBlk := fn.Block(succs[1].Index + 1) | ||
| b.If(cond, thenBlk, elseBlk) | ||
| case *ssa.Panic: | ||
| // Panic in coro version | ||
| val := p.compileValue(b, term.X) | ||
| b.Panic(val) | ||
| } | ||
| } | ||
|
|
||
| return ret | ||
| } | ||
|
|
||
| // compileCoroReturn compiles a Return instruction in coro version. | ||
| // Instead of returning the original value, it jumps to the single exit block | ||
| // where the final suspend is performed. | ||
| func (p *context) compileCoroReturn(b llssa.Builder, ret *ssa.Return) { | ||
| // Jump to the single exit block (which has the final suspend) | ||
| b.Jump(p.coroState.ExitBlk) | ||
| } |
There was a problem hiding this comment.
The functions compileCoroFuncDecl, compileCoroPhis, compileCoroPhi, and compileCoroBlock largely duplicate the logic of the main compilation functions (compileFuncDecl, compilePhis, etc.). This significant code duplication makes maintenance more difficult, as any bug fix or feature addition to the core compilation logic will need to be manually replicated in the coroutine-specific version. Consider refactoring the core compilation functions to be more generic, perhaps by accepting parameters to handle the differences for coroutines, such as a block index offset and a custom handler for return statements. This would reduce code duplication and improve long-term maintainability.
| t.Fatalf("tyCoroEnd should have 2 params, got %d", coroEndSig.Params().Len()) | ||
| } | ||
| if coroEndSig.Results().Len() != 1 { |
There was a problem hiding this comment.
This test for tyCoroEnd expects the signature to have 2 parameters. However, the implementation in ssa/coro.go has been updated to take 3 parameters to support LLVM 19+, as noted in the comment there. The test should be updated to reflect the new 3-parameter signature (ptr, bool, token) to ensure it correctly validates the current implementation.
| t.Fatalf("tyCoroEnd should have 2 params, got %d", coroEndSig.Params().Len()) | |
| } | |
| if coroEndSig.Results().Len() != 1 { | |
| if coroEndSig.Params().Len() != 3 { | |
| t.Fatalf("tyCoroEnd should have 3 params, got %d", coroEndSig.Params().Len()) | |
| } |
Implement on-demand analysis to detect which functions contain suspend points directly or through callees. This enables: - Automatic detection of functions needing $coro versions - Proper await generation when calling tainted functions - Cached results for efficient repeated lookups Add corochain test case to verify deep call chain coroutine handling with multiple nesting levels.
Add test files for verifying that functions with suspend points correctly generate both synchronous and coroutine ($coro) versions. The test demonstrates: - Functions using coroSuspend get dual implementations - Synchronous calls use regular function versions - Asynchronous calls (go statements) use $coro versions
Replace manual pointer load and null comparison pattern with the standard llvm.coro.done() intrinsic to check if a coroutine has completed. This provides a cleaner and more idiomatic approach using the proper LLVM coroutine API.
…nism Implement coroutine return value storage and retrieval using LLVM's Promise mechanism: - Add Promise allocation in coroutine prologue via llvm.coro.id - Store return values to Promise before final suspend - Retrieve values using llvm.coro.promise intrinsic after await - Support single, multiple, struct, and pointer return types - Add comprehensive test cases in cl/_testrt/cororet - Document coroutine implementation design in Chinese
Replace direct returns in coroutine final blocks with branches to the cleanup block containing llvm.coro.end call. This ensures proper coroutine cleanup happens for all exit paths. Also adds cororet test case for coroutine return value handling.
…on report - Move goroutine-vs-llvm-coroutine.md to doc/ - Move llvm-coroutine-impl-guide.md to doc/ - Add comprehensive LLVM coroutine implementation report covering dual-symbol mode, ABI differences, and return value handling strategies
- Add test cases for basic closure calls and variable capture - Implement MakeClosure and ClosureCall in SSA builder - Track actual ending blocks in coroBlkEnds map for proper phi handling - Fix phi node predecessors when suspend points create intermediate blocks - Support closure context passing in coroutine function calls
- Remove explicit coroBlockOn linkname and replace all coroBlockOn(fn) calls with direct fn() invocations in coroutine tests - Add named function types (WorkerFunc, ComputeFunc) and corresponding global variables (typedWorker, typedCompute) - Add new test functions for named function type behavior in async context - Update expected LLVM IR output to reflect the simplified calling pattern
- Add coro mode handling for interface method calls to use $coro versions - Extract common await/load pattern into coroAwaitAndLoadResult helper - Add coroDepth tracking with CoroEnter/CoroExit/CoroIsInCoro runtime functions - Update abiMethodFunc to generate $coro variants for interface methods - Extend IsClosureName to recognize $bound patterns for interface dispatch
Update coroglobal test output to include runtime.CoroEnter() and runtime.CoroExit() calls at coroutine entry and exit points. These calls are inserted after llvm.coro.begin and before coroutine frame cleanup to track coroutine lifecycle in the runtime.
…ections Document implemented coroutine features in the LLVM implementation report: - Section 8: Block_On mechanism for awaiting coroutines and extracting return values from Promise, including sync vs async context handling - Section 9: Closure support with dual-symbol generation and automatic Block_On insertion for closure calls - Section 10: Interface method support with itab population using $coro versions and runtime dispatch - Section 11: Method values and method expressions with $bound$coro and $thunk$coro wrappers - Section 12: Design decision analysis comparing unified $coro approach vs conditional branching, with rationale for chosen design
- Convert closure functions to coroutine variants ($coro suffix) - Add coroutine lifecycle management (CoroEnter, CoroExit, CoroScheduleUntil) - Implement proper coroutine await pattern with suspend/resume loops - Use llvm.coro.promise to retrieve return values from coroutines - Add llvm.coro.destroy for cleanup after coroutine completion
Implement coroutine-based execution model for closure functions: - Add $coro variants of functions with LLVM coroutine intrinsics - Integrate CoroScheduleUntil/CoroEnter/CoroExit runtime calls - Add SkipCoro check to exclude runtime packages from transformation - Update closure test expectations with coroutine implementations
Add a boolean field to closure struct to distinguish between coroutine
and regular function callbacks. This enables proper branching logic to
handle both cases:
- Regular functions are called directly and return values immediately
- Coroutine functions are scheduled and their results extracted from
the coroutine promise
The change updates closure type from {ptr, ptr} to {ptr, ptr, i1} and
adds conditional branching based on the isCoroutine flag throughout
the codebase.
- Add comprehensive module architecture docs (compilation pipeline, package roles, dependency direction) - Document compiler development workflow with debugging steps - Implement coroutine defer mechanism with aCoroDefer and coroDeferStmt types for handling defers in coroutine mode - Add CoroDefer method and getCoroDefer helper for managing defer state in async functions
Add Test 5 to coropanic test suite to verify that panics without recover() properly propagate to the top level. This complements existing tests for panic/recover behavior in coroutine contexts.
Update test output files to reflect new coroutine handling mechanism: - Add boolean flag to function types to distinguish coroutine vs regular calls - Implement runtime branching based on coroutine flag - Add CoroReschedule call before scheduling coroutines - Update promise access with proper GEP for result retrieval
- Add new section 1 explaining push model await implementation - Describe waiter list mechanism in Promise for wake-up coordination - Distinguish sync context (CoroScheduleUntil) vs coroutine context (push await) - Renumber existing sections (1→2, 2→3, etc.) - Simplify Block_On documentation to reflect push/pull model distinction - Remove redundant implementation details and code examples
Restructure and condense the coroutine implementation documentation: - Reorganize sections with clearer hierarchy and numbering - Consolidate dual symbol model explanation as core design - Simplify taint analysis and symbol selection rules - Streamline closure and interface handling documentation - Remove verbose examples while preserving essential concepts - Reduce document size for better readability and maintenance
Reduce the number of basic blocks from 5 to 3 by removing the distinction between first and subsequent wait iterations. The new approach: - Reschedules the callee once at the start - Uses a simple loop to check completion and suspend if needed - Eliminates the phi node for tracking first iteration This simplifies the control flow while maintaining the same await semantics.
- Add early return guard for nil handle - Check if coroutine is already done before enqueueing - Resume coroutine immediately instead of just pushing to queue - Recheck completion status after resume to avoid enqueueing finished coroutines
- Add explicit llvm.coro.resume call immediately after coroutine creation - Remove redundant phi node tracking first-time await status - Consolidate basic blocks by eliminating separate paths for initial vs subsequent awaits - Reduce total number of basic blocks from 17 to 14 per coroutine function
…hSuspend Replace the CoroResume call with CoroReschedule when initiating a coroutine await. This change delegates the rescheduling logic to the dedicated CoroReschedule method, making the intent clearer and removing the need for the explanatory comment.
1 participant
No description provided.