build: Go Linktime-like Unreachable Method Pruning#1736
build: Go Linktime-like Unreachable Method Pruning#1736luoliwoshang wants to merge 18 commits intogoplus:mainfrom
Conversation
|
Well-structured skeleton PR for DCE analysis. The metadata emission infrastructure in |
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| func GetMDString(v llvm.Value) string { | ||
| var n C.uint | ||
| s := C.llgoMDString(unsafe.Pointer(v.C), &n) | ||
| return C.GoStringN(s, C.int(n)) |
There was a problem hiding this comment.
LLVMGetMDString returns NULL when V is not an MDString node. Passing a null s with n > 0 to C.GoStringN will segfault. Consider adding a nil guard:
| return C.GoStringN(s, C.int(n)) | |
| var n C.uint | |
| s := C.llgoMDString(unsafe.Pointer(v.C), &n) | |
| if s == nil { | |
| return "" | |
| } | |
| return C.GoStringN(s, C.int(n)) |
| case "Method": | ||
| p.pkg.EmitReflectMethod(owner) | ||
| case "MethodByName": | ||
| if name, ok := constStr(call.Args[0]); ok { |
There was a problem hiding this comment.
call.Args[0] is accessed without a bounds check. While current Go SSA invariants guarantee one arg for MethodByName(string), a defensive len(call.Args) > 0 guard would prevent panics if the SSA representation is ever malformed. Same applies to line 98 for the static-call branch (call.Args[len(call.Args)-1]).
| ) | ||
| } | ||
|
|
||
| func (p Package) emitMethodOff(owner string, index int, name, mtyp string) { |
There was a problem hiding this comment.
nit: The first parameter is named owner, but here it actually receives a concrete type name (the call site in abiUncommonMethods passes typeName). In the sibling emit functions, owner means "the enclosing function whose reachability triggers the effect." Consider renaming to typeName to match MethodOffRow.TypeName in the dce package and the llgoMethodOffMetadata doc comment.
| } | ||
|
|
||
| func (e *semMetaEmitter) add(mod llvm.Module, table, key string, fields ...llvm.Metadata) { | ||
| fullKey := table + ":" + key |
There was a problem hiding this comment.
nit (perf): fullKey is allocated via string concatenation on every call, even for duplicates. Since this runs on every MakeInterface and Imethod call, consider using a two-level map (map[string]map[string]struct{} keyed by table then key) to avoid the concatenation, or at least defer fullKey construction until after a cheaper preliminary check.
| } | ||
|
|
||
| func (p *context) markReflectMethodCall(call *ssa.CallCommon) { | ||
| owner := p.fn.Name() |
There was a problem hiding this comment.
nit (perf): p.fn.Name() is evaluated unconditionally for every call instruction, even though the vast majority of calls have nothing to do with reflect. Consider moving this assignment inside the conditional branches where owner is actually used, to avoid the string materialization cost on non-reflect calls.
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 introduces the foundational structure for a dead code elimination (DCE) analyzer within the llgo project. It establishes the basic interfaces and data structures required for subsequent development, focusing on setting up the analysis pipeline rather than implementing the core analysis logic. The changes lay the groundwork for future integration with LLVM modules and method liveness computations. 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
|
![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 the initial skeleton for a dead code elimination (DCE) analyzer in the internal/build/dce package. It defines the core data structures and API contracts for the analysis, separating the LLVM-specific parsing from the analysis logic. The PR also implements the crucial metadata emission from the SSA builder, which will be consumed by the DCE analyzer. This includes metadata for interface usage (!llgo.useiface), interface method calls (!llgo.useifacemethod), concrete type method layouts (!llgo.methodoff), and reflection-based method calls (!llgo.reflectmethod, !llgo.usenamedmethod). The changes are well-structured and thoroughly tested, including new test cases for metadata generation and readback. This provides a solid foundation for the subsequent implementation of the DCE analysis. The code quality is excellent, and I have no specific issues to report.
luoliwoshang
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1736 +/- ##
==========================================
+ Coverage 88.44% 88.51% +0.06%
==========================================
Files 50 52 +2
Lines 13656 13815 +159
==========================================
+ Hits 12078 12228 +150
- Misses 1369 1377 +8
- Partials 209 210 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
luoliwoshang
changed the title
luoliwoshang
changed the title
1 participant
Summary
internal/build/dceUsedInIfacechild-type edges, method refs, andllgo.*metadatatype symbol -> live method indexesresults with behavior aligned as closely as practical to Go linkerdeadcode.llcases (closureall,invoke,reflectmethod,reader,ifaceconv)exebuild pipeline and emit strong ABI type overrides that clear deadabi.Methodfunction pointers while keeping type/method layout intact[dce] live methodsduring build so the live method set is directly observableScope
Notes
BuildModeExeTesting
go test ./internal/build/dce -count=1go test ./internal/build -count=1go run ./cmd/llgo build ./cl/_testgo/closureallgo run ./cmd/llgo build /Users/zhangzhiyang/Documents/Code/goplus/llgo/_demo/go/logdemo/log.go