plan9asm: complete ARM64 scalar instruction families - #35
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: ARM64 Go assembler instruction families
Solid, well-structured addition. The new bitfield family (BFI/BFXIL/SBFX/UBFX/SBFIZ/UBFIZ + W variants), ROR/rotate helpers, eval32/setFlagsSub32/setFlagsLogic32, validateARM64MemoryIndex, and the indexed-memory extension parsing were all checked and are correct. In particular:
- ROR /
rotateIntare safe at shift-by-0 (sub bits,0then& (bits-1)yields 0, noshl-by-width poison). - Every new shift/rotate/bitfield emission is range-checked or masked, so no undefined-behavior IR is produced.
- The
parseImmreordering (tryingparseImmExprbefore float parsing) correctly preserves the integer meaning of expressions like$64-31, and the comment accurately explains why.
Security and performance passes found nothing blocking. CI uses pull_request (not pull_request_target) with read-only default permissions; the new shell scripts use set -euo pipefail, mktemp with cleanup traps, quoted vars, and argv-passing to Python; dependencies are version-pinned. The only performance note is the reflection-based fmt.Sprintf("%s %#v", ...) probe key in cmd/plan9asmscan/main.go (scan tooling only, not the lowering path).
A few points below. Note the build could not be run here (LLVM cgo headers are unavailable in this environment), so findings are from static analysis and the PR's own baseline JSON.
Non-blocking notes not placed inline
CMPW computes flags on 64 bits, inconsistent with the new 32-bit SUBSW (arm64_lower_arith.go:666-682, _ = op // CMPW is treated the same as CMP for now.)
This is pre-existing (outside the diff), but the PR adds a correctly 32-bit SUBSW/TSTW path, which makes the inconsistency more visible. CMPW routes both operands through eval64 and computes NZCV via setFlagsSub (64-bit). When the compared registers differ in their high 32 bits, or when the low-32 subtraction borrows/overflows differently than the 64-bit one, C/V (and N/Z) can differ from real CMPW, affecting following HS/LO/HI/LS/GE/LT/GT/LE conditions. Consider routing CMPW through eval32/setFlagsSub32 to match the new SUBSW path. Not introduced by this PR, so non-blocking.
Doc -corpus help / stale snapshot table — see inline notes.
Findings without inline locations
doc/plan9asm-corpus.md:212: The arm64 row of this "Go 1.27 snapshot" table is now stale relative to thego-asm-baseline.jsonthis PR updated. The go1.27 arm64 baseline block reportsunique_forms=1916,supported_forms=455,context_forms=39,unsupported_forms=1422,runtime_verified_forms=58, but this row still shows 1901 / 384 / 21 / 1496 / 0. Since the doc namestestdata/coverage/go-asm-baseline.jsonas the authoritative source, please regenerate this table. In particular the row still claims arm64 "runtime verified: 0" while the PR adds an executable arm64 conformance case and the baseline now records 58.
|
Addressed all review notes in |
|
Extended the authoritative gate in 79b80d0 to the complete LLGo Plan 9 GOARCH surface: 386, amd64, arm, arm64, and wasm. The Linux/wasm standard-library matrix now expands across every architecture setting supported by the selected Go release, while the cross-version fingerprint includes wasm via its official opcode namespace and concrete GOROOT assembly (Go has no wasm operand-class encoder table). I verified the official inventory on the latest patch releases from Go 1.20 through Go 1.27, ran representative old-version level matrices, and ran the full Go 1.27 44-target scan/translate/LLVM-object matrix locally in 352 seconds with zero unsupported forms or parse failures. The broader completion criteria are recorded in #36. |
|
The first expanded CI run identified a host/target separation bug in the gate itself: go run rebuilt plan9asmscan under GOAMD64=v4, so GitHub runners without v4 host CPUs refused to start the scanner before any target corpus was checked. ad79ff0 builds the host scanner once before applying target settings, then passes those settings only to the scanner process and its target go list calls. This is the correct cross-compilation model and also removes 44 repeated scanner builds; the complete local 386 plus GOAMD64 v1-v4 matrix now passes in 55 seconds. |
zhouguangyuan0718
left a comment
zhouguangyuan0718
left a comment
There was a problem hiding this comment.
Two P1 correctness issues reproduced at ad79ff0 by running the same assembly through native Go and plan9asm-generated LLVM on macOS/arm64 with Go 1.26.5. The existing go test ./... and all three ARM64 conformance tests pass, but the additional cases below produce different results. Please fix both and add executable conformance regressions before merging.
3 participants
Summary
Complete the ARM64 scalar instruction and operand families exposed by the Go assembler corpus around xgo-dev/llgo#2552, including the bitfield aliases, conditional-select aliases, 32/64-bit shifts and reversals, flag-setting forms, signed and unsigned narrow moves with shifted or extended indexed addressing, and FMOVQ memory forms. The implementation is defined by Go's ARM64 encoder tables and official positive assembler tests, not by a third-party package.
Add an executable semantic oracle that runs the same ARM64 assembly through the native Go assembler and through plan9asm-generated LLVM, covering 59 normalized forms and checking 72 exact results. The Go 1.20 through Go 1.27 corpus gate now requires every one of the 54 completed official opcodes to be encoder-defined, observed in official positive testdata, and free of unsupported observed operand forms.
Add the complete pinned golang.org/x/arch/arm64/arm64asm Plan 9 decoder corpus as a larger supplemental source. Because decoder output can include reserved encodings or forms rejected by the Go assembler, the completed-family subset is filtered through native go tool asm before plan9asm support is required.
Use github.com/klauspost/compress v1.20.0 only as the final external regression consumer: all 14 amd64/arm64 assembly files are translated and LLVM-compiled after the authoritative gates pass. With latest llgo main and this branch connected by go.work, LLGO_PLAN9ASM_PKGS=all llgo test github.com/klauspost/compress/huff0 and github.com/klauspost/compress/zstd/internal/xxhash both pass, and a huff0 compression/decompression round trip matches native Go.
Extend the durable coverage gate to all five independent GOARCH targets used by LLGo's Plan 9 path: 386, amd64, arm, arm64, and wasm. The standard-library gate now expands Linux and wasm targets across all settings supported by each selected Go release, including GO386 sse2/softfloat, GOAMD64 v1-v4, GOARM 5/6/7 and available float ABIs, GOARM64 v8.0-v9.5 plus lse/crypto boundaries, and all GOWASM satconv/signext combinations for js/wasm and wasip1/wasm. WebAssembly uses Go's official opcode namespace plus concrete GOROOT assembly because its backend has no x86/ARM-style operand-class encoder table; those opcode-only rows are explicitly reported rather than presented as encoder forms. Closes #36.
Addresses xgo-dev/llgo#2552.
Validation