plan9asmll: lower aggregate signatures for llgo cabi#6
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 refines how Go aggregate types are translated into LLVM signatures for llgo's cabi compatibility, particularly for the ARM64 architecture. The changes introduce a more robust, declaration-driven system for handling types like strings, slices, and interfaces, ensuring accurate representation in LLVM. It also improves ARM64's handling of aggregate arguments and provides a crucial fallback mechanism for return values, enhancing the correctness and extensibility of the plan9asmll toolchain. 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 introduces a significant and well-executed refactoring of the plan9asm to LLVM IR translation for ARM64. The changes to lower aggregate type signatures (string, slice, interface) based on their Go declarations make the translator more robust and extensible. The addition of a fallback to register-based returns when result slots are not explicitly written is a key improvement for correctness. The new tests and design document are valuable additions. I have a couple of minor suggestions to reduce code duplication.
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| func (c *arm64Ctx) loadRetSlotFallback(slot FrameSlot) (string, error) { | ||
| if slot.Index < 0 || slot.Index > 31 { | ||
| return llvmZeroValue(slot.Type), nil | ||
| } |
There was a problem hiding this comment.
slot.Index is a result index, not a physical register number. Mapping it directly to R{Index} works only if the calling convention happens to place the Nth result in R{N} — which holds today for single-result functions, but is an undocumented invariant. For multi-result functions this becomes fragile. Consider either documenting the invariant explicitly or deriving the fallback register through the same mechanism used for parameters.
| } | ||
|
|
||
| func (c *arm64Ctx) loadRetSlotFallback(slot FrameSlot) (string, error) { | ||
| if slot.Index < 0 || slot.Index > 31 { |
There was a problem hiding this comment.
Returning a zero value silently when slot.Index > 31 masks misconfigured frame layouts. A zero return is a valid result, making this indistinguishable from correct behavior. Returning an error would make bugs in signature inference much easier to diagnose.
| for fi, fty := range fields { | ||
| r := Reg(fmt.Sprintf("R%d", *regCursor)) | ||
| *regCursor++ | ||
| v, err := c.loadReg(r) |
There was a problem hiding this comment.
castI64RegToArg handles integer types and ptr, but not double or float. Functions with floating-point parameters will hit the default error branch and fail to translate. By contrast, loadRetSlotFallback (and the old inline switch) both handle double/float. This omission will break any callee with FP parameters declared in sigs.
| v, err := c.loadReg(r) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
When len(csig.ArgRegs) > 0 but i >= len(csig.ArgRegs), the fallback uses the loop index i as the register number rather than regCursor. This is inconsistent with tailCallAndRet (which uses regCursor for overflow args) and won't account for registers already consumed by aggregate arguments earlier in the argument list.
| } | ||
|
|
||
| args := make([]string, 0, len(csig.Args)) | ||
| regCursor := 0 |
There was a problem hiding this comment.
The useLLVMArgs signature-equality check (which compares callee and caller Args slices) is loop-invariant — its result is the same for all iterations of i. Moving it outside the loop avoids an O(N²) comparison for N-argument callees.
|
Good refactor — replacing the ad-hoc
|
1 participant
Summary
Testing