Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
cpunion
changed the title
LLGo baseline benchmarks
Program measurements
Core language and compiler benchmarks
Timer runtime benchmarks
Compared with |
cpunion
marked this pull request as ready for review
September 30, 2026 07:50
cpunion
changed the title
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review summary
PR #2694 ("WIP: Diagnose modernc.org/libc ABI0 wrappers", #2691) adds one translation test plus a replace that redirects github.com/xgo-dev/plan9asm to a personal fork. The test is well-structured and consistent with the package's existing tests (mustTestPackage, TranslateSourceModuleForPkg, defer tr.Module.Dispose(), llvm.VerifyModule), and the ABI0 frame arithmetic ($56-52) is internally consistent. The two concerns are the committed fork redirect and a brittle/under-specified IR assertion — both inline below.
Findings: 1 merge-relevant (go.mod fork replace), 2 test-quality. No performance issues found.
Not placeable inline (outside the diff): THIRD_PARTY_NOTICES.md:70 still lists only github.com/xgo-dev/plan9asm, while the effective module graph (go.sum) now builds github.com/cpunion/plan9asm — the provenance notice no longer matches what ships.
Not buildable locally (LLVM headers unavailable), so this was judged by reading, not by running.
|
|
||
| replace github.com/xgo-dev/llgo/runtime => ./runtime | ||
|
|
||
| replace github.com/xgo-dev/plan9asm => github.com/cpunion/plan9asm v0.0.0-20260929013439-34a93b2081cf |
Contributor
There was a problem hiding this comment.
P1 — a committed replace redirects a first-party compiler dependency to a personal fork. This shadows the github.com/xgo-dev/plan9asm v0.6.1 requirement for every build of the module (not only this test), moving control of compiled codegen from the xgo-dev org to an untagged personal-fork pseudo-version (v0.0.0-...). The shipped code differs from the org release — the module zip hashes differ (fork h1:6gLAPwaW... at go.sum:1 vs org v0.6.1 h1:WcwHdcNK... at go.sum:29) — so this is an unreviewed divergence on a compiler path, not a no-op. The replace also silently overrides any future go get -u of the real module until removed. For a WIP diagnostic, prefer a non-committed override (e.g. go.work) or upstream the fork and bump the require to a tagged release before merge; the fork redirect should not land in committed go.mod.
Collaborator
Author
There was a problem hiding this comment.
Agreed: this replacement must not land on main, and I am leaving this P1 unresolved. I verified the latest official release is still v0.6.1 and reran the regression using a separate modfile without the fork override: it fails with unsupported struct type struct{Lo uint64; Hi uint64}. Removing the override now would break the regression; skipping it would hide the gap. The PR body now explicitly gates merge on an upstream dependency containing the fix, together with the API/ABI0 integration and execution tests. 71d85c0 also addresses the separate provenance finding in THIRD_PARTY_NOTICES.md; the pinned fork LICENSE is byte-identical to the bundled Plan9Asm license. This documentation does not resolve the dependency blocker.
LLGo WebAssembly build benchmarks
WebAssembly output sizes
LLGo WebAssembly build measurements
Compared with |
cpunion
marked this pull request as draft
October 1, 2026 02:30
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.
Refs #2691. This is a dependency-integration experiment, not a complete SQLite or ABI0 fix and not ready to merge.
Scope
Merge blockers
unsupported struct type struct{Lo uint64; Hi uint64}when tested with a separate modfile. The replacement affects all LLGo builds, not only tests, and must not land on main.Validation
go test ./internal/plan9asm -count=1go test ./internal/plan9asm -race -count=1go vet ./internal/plan9asm