Conversation
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review: single unsigned bounds compare
The change replaces the two-part bounds check (icmp slt idx, 0 OR icmp uge idx, max) with a single unsigned idx >=u max, backed by the invariant that max is a non-negative len/cap so signed negatives fail as large unsigned values.
I traced correctness across all paths and it holds:
- Negative signed indexes are sign-extended by
toNative/castIntbefore the compare, so they become large unsigned values and are rejected. - Extended 64-bit-index / 32-bit-word path still compares on the widened i64 operands (via
boundsArg) and defers truncation until after the check, preventing an out-of-range uint64 from truncating into an apparently-valid index.TestWideIndexBoundsCheck386still guards this. - Panic reporting stays correct:
PanicIndex/PanicIndexUselection is preserved and the raw index is reported. indexNeedsCheckonly elides the check for a constant index provably in[0, max)against a constant max — every non-constant or unprovable case conservatively returnstrue.
Net effect is fewer IR instructions per bounds check (drops one icmp and one or i1 on the common path) with no correctness or performance regression. Fixture CHECK/CHECK-NOT directives and the new TestSignedIndexUsesUnsignedCompare accurately pin the new lowering.
Minor / optional (non-blocking):
ssa/datastruct.go— thecheckIndexdoc comment (the "single unsigned comparison idx >=u max" note) describes only the common path; the extended 64-bit/32-bit path below preserves the same invariant but isn't mentioned. A one-line note there would help future readers.
Two small suggestions are left inline. No blocking issues.
len/cap is non-negative, so idx >=u max already rejects signed negatives. Drop the extra icmp slt / or on the index fast path to match Go.
visualfc
force-pushed
the
improve/index-unsigned-compare
branch
from
September 15, 2026 14:00
e39eae0 to
89fc757
Compare
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.
Summary
idx >=u lencompare.len/capare non-negative, so the extraicmp slt/oron the fast path is redundant.PanicIndexvsPanicIndexU(and the 32-bit wide-index split) so panic text is unchanged.This matches Go's
IsInBoundslowering.Tests
ssa.TestSignedIndexUsesUnsignedComparechecks signed indexes emiticmp ugewithouticmp slt.cl/_testgo/indexerrIR checks updated for the single compare.concat/tprecursnapshots refreshed.Performance
Isolated index-check microbenchmarks on darwin/arm64 (Apple M5, 1s × 6). These avoid LLGo's noinline
[]Tstack-header ABI, so they compare only the bounds-check lowering.s[idx]whereidxis loaded, so LLVM cannot proveidx >= 0. Baseline keeps an extratbnz; this PR is a singleuge.s[i&4095]. Both compilers already fold to oneuge.data[i]withlenloaded from a global (L1), not a caller stack store.A noinline
[]Tcall can still look slower because LLGo passes the slice header on the stack; that is ABI/store-forwarding, not this compare.