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 enhances the LLGo compiler's SSA generation by adding a mechanism to emit semantic metadata. Specifically, it now tracks and annotates instances where concrete types are converted to interface types using 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. Footnotes
|
|
Overall: Clean, well-scoped PR. The semantic metadata emitter is a solid foundation for recording interface conversions. The golden file updates are consistent. A few items below worth addressing — mainly a redundant |
ssa/interface.go
Outdated
| if _, ok := types.Unalias(typ.raw.Type).Underlying().(*types.Interface); !ok { | ||
| b.Pkg.emitUseIface(b.Func.impl, b.abiTypeGlobal(typ.raw.Type).impl) | ||
| } | ||
| tabi := b.abiType(typ.raw.Type) |
There was a problem hiding this comment.
Performance: abiTypeGlobal(typ.raw.Type) is called here at line 121, then called again at line 123 via abiType (which internally calls abiTypeGlobal with the same argument). On first invocation per type this performs TypeName resolution + VarOf map lookup + potentially heavy global construction; on subsequent calls it still does the name computation and lookup.
Consider hoisting the call and reusing the result:
| if _, ok := types.Unalias(typ.raw.Type).Underlying().(*types.Interface); !ok { | |
| b.Pkg.emitUseIface(b.Func.impl, b.abiTypeGlobal(typ.raw.Type).impl) | |
| } | |
| tabi := b.abiType(typ.raw.Type) | |
| if _, ok := types.Unalias(typ.raw.Type).Underlying().(*types.Interface); !ok { | |
| b.Pkg.emitUseIface(b.Func.impl, b.abiTypeGlobal(typ.raw.Type).impl) | |
| } | |
| tabi := b.abiType(typ.raw.Type) |
→
g := b.abiTypeGlobal(typ.raw.Type)
if _, ok := types.Unalias(typ.raw.Type).Underlying().(*types.Interface); !ok {
b.Pkg.emitUseIface(b.Func.impl, g.impl)
}
tabi := Expr{llvm.ConstGEP(g.impl.GlobalValueType(), g.impl, []llvm.Value{...}), prog.AbiTypePtr()}Or at minimum, ensure abiType can accept a precomputed Global to avoid the double lookup.
| @@ -0,0 +1,51 @@ | |||
| package ssa | |||
There was a problem hiding this comment.
Consistency: This file is missing the Apache 2.0 copyright header that every other .go file in ssa/ includes. Please add it for consistency:
/*
* Copyright (c) 2024 The XGo Authors (xgo.dev). All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* ...
*/
ssa/metadata.go
Outdated
| func (p Package) emitUseIface(owner, target llvm.Value) { | ||
| if owner.IsNil() || target.IsNil() { | ||
| return |
There was a problem hiding this comment.
Robustness: The silent nil-guard here may mask bugs upstream. If b.Func.impl or the result of abiTypeGlobal is unexpectedly nil, this silently drops the metadata rather than surfacing the problem. Consider at least a debug-mode log/warning so these cases don't go unnoticed during development.
ssa/metadata.go
Outdated
| p.semMeta.add( | ||
| p.mod, | ||
| llgoUseIfaceMetadata, | ||
| metadataKey(owner.Name(), target.Name()), | ||
| metadataSymbol(owner), |
There was a problem hiding this comment.
Clarity / Correctness: The dedup key is built from owner.Name() and target.Name(). Two concerns:
- Key collision risk: If two distinct LLVM values share the same name (or both have empty names), the deduplication would incorrectly suppress a legitimate entry.
- A brief comment explaining the
\x00separator choice inmetadataKeyandaddwould help future maintainers understand this is safe because it cannot appear in symbol names.
1958ac9 to
8e77b05
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces an infrastructure for emitting package-level semantic metadata in the SSA backend and uses it to emit !llgo.useiface metadata for non-interface to interface conversions. The implementation includes a new semanticMetadataEmitter to manage and prevent duplicate metadata entries in the LLVM module. The changes are well-structured, with a logical refactoring in abitype.go to support the new functionality in interface.go. The code is clean, correct, and the regenerated golden files confirm the intended behavior. Overall, this is a solid improvement to the compiler's metadata emission capabilities.
57e867d to
b0f7c04
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1728 +/- ##
==========================================
+ Coverage 93.15% 93.19% +0.03%
==========================================
Files 48 50 +2
Lines 13352 13511 +159
==========================================
+ Hits 12438 12591 +153
- Misses 727 731 +4
- Partials 187 189 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
646581d to
1b2e89b
Compare
728feba to
a4b76a5
Compare
There was a problem hiding this comment.
will use goplus/llvm instead
3e35f66 to
7222776
Compare
7222776 to
146a83e
Compare
3064a49 to
b2b62bb
Compare
b2b62bb to
6cad6b2
Compare
# Conflicts: # cl/_testgo/abimethod/out.ll # cl/_testgo/embedunexport-1598/out.ll # cl/_testgo/interface/out.ll # cl/_testgo/interface1370/out.ll
There was a problem hiding this comment.
接口方法数组: 0 0 0 0
类型。 1 1 0 1
Summary
!llgo.useiface,!llgo.useifacemethod, and!llgo.methodoffchore/gentestsValidation
env GOCACHE=/tmp/llgo-gocache go test ./ssa -run '^$'env GOCACHE=/tmp/llgo-gocache go run ./chore/gentestsenv GOCACHE=/tmp/llgo-gocache go test ./cl -run 'TestFromTestgo/(ifaceconv|invoke|abimethod|interface)$' -count=1