refactor: reuse LLVM function callees through the LLVM 22 API - #2517
Open
zhouguangyuan0718 wants to merge 1 commit into
Open
zhouguangyuan0718 wants to merge 1 commit into
zhouguangyuan0718 wants to merge 1 commit into
Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: LLVM C API cleanup
The source refactor is clean and correct: replacing the recurring NamedFunction + IsNil + AddFunction idiom with a single GetOrInsertFunction call removes ~14 lines of boilerplate across three sites with no behavioral change. All three converted call sites use the result only as a callee (passing an explicit fnType to CreateCall) or as an opaque function-pointer operand, and none read Function-specific attributes — so the new API's "check IsAFunction before accessing function attributes" caveat does not apply here. Good, faithful consolidation.
The one substantive concern is the dependency change, not the Go logic (see inline comments).
Informational (no action): GetOrInsertFunction can, per its own contract, return a non-Function value (alias or a constant-expression cast under typed pointers) if a symbol of the same name already exists with a different type. This codebase uses opaque pointers and no converted site reads function attributes, so it is safe today; it would only matter if these helpers were reused under a typed-pointer configuration.
Additional findings
/workspace/llgo/THIRD_PARTY_NOTICES.md:69: [P3] THIRD_PARTY_NOTICES.md does not reflect the fork redirect:THIRD_PARTY_NOTICES.mdstill lists onlygithub.com/xgo-dev/llvmand no longer names the repository actually vendored via the newreplace. Low severity and arguable (the declared module path is unchanged and the redirect may be temporary), but worth a note if the fork is intended to persist. This also naturally goes away if the change is upstreamed and thereplacedropped.
|
|
||
| replace github.com/xgo-dev/llgo/runtime => ./runtime | ||
|
|
||
| replace github.com/xgo-dev/llvm => github.com/zhouguangyuan0718/go-llvm v0.0.0-20260906105604-980fcc6eec0e |
Contributor
There was a problem hiding this comment.
[P1] replace directive points a core dependency at a personal fork
This redirects the LLVM binding — a core build-time dependency that generates and links native code — to a personal fork (zhouguangyuan0718/go-llvm) pinned to an untagged pseudo-version.
Concerns:
- Build fragility / trust boundary: the build now depends on an individual account's repo, which can be renamed, made private, force-pushed, or deleted, breaking every future build and moving a core dependency outside the project's trust boundary.
- Cannot compile against the declared upstream:
GetOrInsertFunctionexists only in this fork; it is absent fromgithub.com/xgo-dev/llvm v0.9.9still listed inrequire. Sorequireandreplacedisagree about what actually builds.
A replace to a personal fork is fine for local/draft development but should not land on a shared branch. Recommendation: upstream GetOrInsertFunction into xgo-dev/llvm, cut a tagged release, bump the require, and drop the replace before merging.
| github.com/xgo-dev/plan9asm v0.5.2 h1:P5eGHZiuKNcL2tlkkICa2XcxghUSnp6QzfN52x0HW1g= | ||
| github.com/xgo-dev/plan9asm v0.5.2/go.mod h1:ZzybDk59y/AXoQeYUEqxVkKWqalmCfmcfCtM9y85kQU= | ||
| github.com/zhouguangyuan0718/go-llvm v0.0.0-20260906105604-980fcc6eec0e h1:k6oC0irlMkDRyun6dX8gebXo7OmjzpHHBrHuvVLTxJU= | ||
| github.com/zhouguangyuan0718/go-llvm v0.0.0-20260906105604-980fcc6eec0e/go.mod h1:42vav2/cI5BAIcL543DZSMO9do8/aCK2z7JERH+AE+M= |
Contributor
There was a problem hiding this comment.
[P2] fork go.mod hash matches original, masking the substitution
The fork's go.mod hash (h1:42vav2/...) is byte-for-byte identical to github.com/xgo-dev/llvm v0.9.9. Because both go.mod files hash the same, nothing in the module graph visibly signals that the underlying source was swapped — only the differing zip (h1:) content hash reveals it, which is easy to overlook in a go.sum review. Reviewers should diff the fork's source against the upstream release rather than rely on go.sum here. This resolves once the dependency is upstreamed and the replace removed.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use
Module.GetOrInsertFunctionfor the large-aggregate allocator and DCE override function references, replacing three separate function lookup/create sequences. Existing declarations are reused through LLVM 22's C API, including LLVM's handling of pre-existing callees. Sites that intentionally create new functions remain onAddFunction.Depends on xgo-dev/llvm#55. Temporarily pin the binding to
github.com/zhouguangyuan0718/go-llvm v0.0.0-20260906105604-980fcc6eec0e; replace this personal-fork pin with the upstream release after the binding lands. The companion binding PR also adds non-consumingParseIRBufferwith older-LLVM compatibility; LLGo has no productionParseIRcallers to migrate.Validation:
go test ./internal/abi ./internal/dcepasspassed on macOS arm64 with LLVM 22.1.8 using the pinned remote module. The binding's full LLVM 22 suite and focused LLVM 21/19 compatibility tests passed locally; remote CI remains pending.