Repository navigation
internal/loopir: reject a constant offset that puts an index below zero - #2
Merged
Merged
Conversation
A read at a negative constant offset in a loop with a constant limit, such as dst[i] = a[i-20] + 1 in a loop that stops before 16, was rewritten into code that does not compile: the length check of the vector loop is _ = a[16-21], a negative constant index. -split wrote that file, and the toolexec wrapper fell back to the original package. Such a loop panics as soon as it runs, as does one whose first index is a negative constant (a[i-1] in a loop from 0). Both are now left alone with their own reason, the way a constant limit of zero or less already is. A read whose indexes can all be zero or more (a[i-1] from 1) is still rewritten. The checks read the start and limit as text, so a named constant or a constant expression (const s0 = 0; for i := s0; ...) slipped past them. A constant start or limit is now spelled as its value, which also lets the short-loop rule see a named constant trip count: goav1's for x := range restorationExtraHorz, three iterations, is no longer rewritten. The vectorized set of std and gonum does not change.
thesyncim
force-pushed
the
fix-negative-constant-offset
branch
from
November 1, 2026 13:36
087660e to
00263c0
Compare
This was referenced Nov 1, 2026
Closed
Open
mauri870
enabled auto-merge (squash)
November 3, 2026 20:59
mauri870
added a commit
that referenced
this pull request
Nov 3, 2026
) Follow-up to #2. #2 started replacing every constant start or limit with its value, so that the checks parsing `Trip.Start`/`Trip.Limit` as text would also see named constants. The emitted file is compiled for every build, though, and a named constant can have a different value in another one. With `const width = 32` for amd64 and `64` for arm64, `loopvec -split` on amd64 hard-coded `32` into the vector loop of `for i := 0; i < width; i++`, while the scalar fallback kept `width`. The arm64 build would then vectorize only half the elements. `plusOne` and `i > low` loops had always done the same for `<=` limits and loops that count down. What changes: - **Constant names are kept.** `Trip` carries the values of a constant start and limit. The checks read those values instead of parsing text: the negative-index check, the short-loop rule and the copy check. A constant is written as its value only when it's built from literals alone (`16`, `1 - 1`). A constant that uses a name keeps the name. `i <= width-1` comes out as `width`. - **Dead code removed.** `negativeSource` could no longer return true after #2. - **The negative-index check can't overflow.** It negates the offset instead of adding it to the bound. It also runs in the same pass over the loop body as the stored-offset checks. - **Bounds pushed past the largest int are rejected.** A positive offset that pushes a constant start or limit past the largest int is rejected as that bound. `for i := 0; i < math.MaxInt64; i++ { dst[i] = a[i+1] }` used to produce an overlap window of `math.MaxInt64 + 1`, which doesn't compile. - **Tests:** - `offset_negative_constant.txt` now checks the rewritten source and runs the loops under both builds. - The rejected loops moved to `offset_negative_constant_no_rewrite.txt`, which adds a loop counting down to 0, a range over a slice, and the two loops pushed past the largest int. - `bound_arch_constant.txt` covers the GOARCH case for `<`, `<=`, counting down and `>`. - **AGENTS.md** documents the new rejections and the rule for keeping names. Tested locally: - `bound_arch_constant.txt` fails against #2's `internal/loopir`, which writes `_ = dst[32-1]` where the test expects `_ = dst[width-1]`. - `make test` and `make test-qemu-arm64` pass. - `make test-update` and `make fix` leave no diff, and `make lint` reports 0 issues. - `loopvec -json` gives identical output before and after over std (1663 loops) and gonum (3252 loops).
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
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.
loopvec writes a
_simd.gothat doesn't compile for a loop like this:The length check comes out as
_ = a[16-21]:toolexec falls back, but
-splitand-wleave the broken file behind.A loop like that panics as soon as it runs, and so does one whose first index is a negative constant (
a[i-1]starting at 0). Both are now rejected with their own reason, the same way a constant limit of zero or less already is.a[i-1]starting at 1 is still rewritten.The checks parsed the start and limit as text, so named constants and constant expressions (
const s0 = 0; for i := s0; ...,for i := 1 - 1; ...) slipped through. Constant bounds are now written out as their value. A side effect is that the short-loop rule now sees named constant trip counts too: in goav1, a 3-iterationfor x := range restorationExtraHorzis no longer vectorized. The vectorized set of std and gonum doesn't change.