cl/ssa: split invoke through reusable thunk#1669
cl/ssa: split invoke through reusable thunk#1669luoliwoshang wants to merge 1 commit intogoplus:mainfrom
Conversation
Summary of ChangesHello, 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 significantly refactors the internal handling of interface method calls within the LLGo compiler. By introducing reusable thunk functions, the system moves towards a more standardized and potentially optimized approach for dispatching non-variadic interface methods, streamlining the generated LLVM IR and improving code generation consistency. 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. Changelog
Activity
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 refactors interface method calls to use reusable thunks, which is a good optimization for reducing code size. The implementation introduces weak_odr LLVM functions for non-variadic interface method invocations. The logic for creating these thunks and modifying the call sites appears correct and is well-structured. The changes are consistently applied across the codebase, as reflected in the updated test outputs. The decision to handle variadic functions separately for now is a reasonable incremental approach. Overall, this is a solid improvement.
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| @@ -79,6 +80,59 @@ func (b Builder) Imethod(intf Expr, method *types.Func) Expr { | |||
| sig = types.NewSignatureType(nil, nil, nil, types.NewTuple(vars...), sig.Results(), sig.Variadic()) | |||
| } | |||
| } | |||
| return sig | |||
| } | |||
There was a problem hiding this comment.
invokeMethodSig performs a non-obvious transformation (stripping the implicit interface receiver from the first parameter position when there is no explicit Recv). A brief doc comment would help future readers understand the contract — particularly when the signature is returned unmodified (method has a receiver, or first param doesn't match the interface).
This function is now shared between both Imethod and InvokeThunk, making it an important piece of the interface dispatch logic.
| func (p Package) invokeThunk(rawIntf *types.Interface, method *types.Func, sig *types.Signature) Function { | ||
| methodName := method.Name() | ||
| methodIdx := iMethodOf(rawIntf, methodName) | ||
| if methodIdx < 0 { | ||
| panic("invokeThunk: interface method not found: " + methodName) | ||
| } | ||
| name := p.invokeThunkName(rawIntf, methodName, methodIdx) | ||
| if thunk := p.FuncOf(name); thunk != nil { | ||
| return thunk | ||
| } |
There was a problem hiding this comment.
On the cache-hit path, invokeThunk still runs iMethodOf (linear scan) at line 93, invokeThunkName at line 97 (which calls InterfaceName → SHA-256 hash + base64 encode), and string concatenation — all before the FuncOf lookup at line 98 discards the result because the thunk already exists.
For hot interfaces invoked at many call sites (e.g., io.Reader, error), this repeated work could add up at compile time. Consider caching by a cheaper key (e.g., (*types.Interface, string) composite) or caching the InterfaceName result.
| args = append([]llssa.Expr{o}, args...) | ||
| ret = b.Do(act, fn, args...) |
There was a problem hiding this comment.
Nit: append([]llssa.Expr{o}, args...) allocates a 1-element slice then grows it. Since args is freshly allocated by compileValues, you could pre-allocate with the right capacity:
| args = append([]llssa.Expr{o}, args...) | |
| ret = b.Do(act, fn, args...) | |
| fullArgs := make([]llssa.Expr, 0, len(args)+1) | |
| fullArgs = append(append(fullArgs, o), args...) | |
| ret = b.Do(act, fn, fullArgs...) |
|
Clean refactoring that replaces per-call-site itab dispatch with a shared Main suggestions: (1) add a doc comment to |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1669 +/- ##
==========================================
+ Coverage 91.35% 91.36% +0.01%
==========================================
Files 47 47
Lines 12681 12720 +39
==========================================
+ Hits 11585 11622 +37
- Misses 906 907 +1
- Partials 190 191 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
ssa/interface.gowith weak_odr linkage and stable naming (__llgo_invoke.<iface>$m<idx>.<method>)Imethodpath for now to avoid varargs forwarding regressions.llexpectations withgentestValidation
go test ./ssa -run 'TestIfaceMethodClosureCallIR|TestInterfaceHelpers' -count=1go test ./ssa -run 'TestFromTestgo/(abimethod|invoke|ifaceprom)|TestFromTestrt/(tpmethod|vamethod)' -count=1go test ./ssa -run 'TestFromTestgo/(interface|reader)|TestFromTestrt/(mapclosure)' -count=1go run ./chore/gentestsgo install ./cmd/llgo