From 9a80d9c224363ce3a89ea4b0ce29b51123f27a09 Mon Sep 17 00:00:00 2001 From: Marcelo Pires Date: Tue, 3 Nov 2026 21:05:19 +0000 Subject: [PATCH 01/10] internal/loopir: reject a constant offset that puts an index below zero (#2) loopvec writes a `_simd.go` that doesn't compile for a loop like this: ```go for i := 0; i < 16; i++ { dst[i] = a[i-20] + 1 } ``` The length check comes out as `_ = a[16-21]`: ``` invalid argument: index 16 - 21 (constant -5 of type int) must not be negative ``` toolexec falls back, but `-split` and `-w` leave 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](https://github.com/thesyncim/goav1), a 3-iteration `for x := range restorationExtraHorz` is no longer vectorized. The vectorized set of std and gonum doesn't change. Co-authored-by: Mauri de Souza Meneguzzo --- internal/loopir/lower.go | 25 ++++++++ internal/loopir/reason.go | 6 ++ testdata/offset_negative_constant.txt | 90 +++++++++++++++++++++++++++ 3 files changed, 121 insertions(+) create mode 100644 testdata/offset_negative_constant.txt diff --git a/internal/loopir/lower.go b/internal/loopir/lower.go index c4a7719..8d6eb6e 100644 --- a/internal/loopir/lower.go +++ b/internal/loopir/lower.go @@ -286,6 +286,12 @@ func (l *lowerer) bound(e ast.Expr) (bound, bool) { } b := bound{text: types.ExprString(e)} b.value, b.isConst = l.constant(e) + if b.isConst { + // A named constant or a constant expression (s0, 1 - 1) is spelled as its + // value, so the checks that read a constant start or limit see it and the + // emitted code never indexes at a negative constant they would have caught. + b.text = strconv.FormatInt(b.value, 10) + } if call, ok := ast.Unparen(e).(*ast.CallExpr); ok { b.slice, _ = l.lenOf(call) } @@ -532,6 +538,25 @@ func (l *lowerer) body(node ast.Stmt, block *ast.BlockStmt, trip Trip, step int6 } } + // A constant offset that puts an index below zero makes the scalar loop panic + // when it runs: on its first iteration when the start is a constant and the + // first index is negative, or on every iteration when the limit is a constant + // and even the last index is. It is left alone, since the vector loop would + // index or slice at a negative constant, which does not compile. + start, errStart := strconv.ParseInt(trip.Start, 10, 64) + limit, errLimit := strconv.ParseInt(trip.Limit, 10, 64) + negative := func(offset string) bool { + off, err := strconv.ParseInt(offset, 10, 64) + return err == nil && (errStart == nil && start+off < 0 || errLimit == nil && limit+off <= 0) + } + for _, stmt := range body { + for _, leaf := range Leaves(stmtValue(stmt)) { + if load, ok := leaf.(*Load); ok && negative(load.Off) { + return nil, ReasonNegativeIndex + } + } + } + loop := &Loop{ Node: node, Ind: Induction{Var: l.iv, Step: step, Trip: trip}, diff --git a/internal/loopir/reason.go b/internal/loopir/reason.go index aaeca53..4757287 100644 --- a/internal/loopir/reason.go +++ b/internal/loopir/reason.go @@ -42,6 +42,12 @@ const ( // iteration writes (a[i] = a[i-1] + x): iteration i reads what iteration i-1 // wrote, so the iterations cannot run as a vector. ReasonCarriedDependence Reason = "an iteration reads an element an earlier iteration wrote" + // ReasonNegativeIndex covers a slice read at a constant offset that puts an + // index below zero: the first index, with a constant start (a[i-1] in a loop + // from 0), or every index, with a constant limit (a[i-20] in a loop that stops + // before 16). The scalar loop panics when it runs, and the vector loop would + // index or slice at a negative constant, which does not compile. + ReasonNegativeIndex Reason = "a constant offset puts an index below zero" // ReasonUnsupportedIf covers an if that is not a select: a condition that is // not a comparison of loop values (joined by && and ||), an init statement, or // a branch that is not one assignment to an element of the same slice. diff --git a/testdata/offset_negative_constant.txt b/testdata/offset_negative_constant.txt new file mode 100644 index 0000000..d23cc26 --- /dev/null +++ b/testdata/offset_negative_constant.txt @@ -0,0 +1,90 @@ +# A read at a negative constant offset is rewritten while every index it reads can +# be zero or more. When an index is a negative constant, the first one in a loop +# with a constant start (a[i-1] from 0) or every one in a loop with a constant limit +# (a[i-16] in a loop that stops before 16), the scalar loop panics when it runs and +# the vector loop would index at a negative constant, which does not compile, so the +# loop is left alone, whether the start and limit are written as literals, named +# constants or constant expressions. The rewritten package must build. + +loopvec -json . +cmpenv stdout stdout.want + +loopvec -split . +cmpenv stderr stderr.want +env GOEXPERIMENT=simd +exec go build . + +-- go.mod -- +module example.com/test + +go 1.27.1 +-- ops.go -- +package test + +func FirstIndexZeroConstant(dst, a []int32) { + for i := 15; i < 31; i++ { + dst[i] = a[i-15] + 1 + } +} + +func EveryIndexNegative(dst, a []int32) { + for i := 0; i < 16; i++ { + dst[i] = a[i-16] + 1 + } +} + +func FarBelow(dst, a []float32) { + for i := 0; i < 16; i++ { + dst[i] = a[i-20] * a[i] + } +} + +func FirstIndexNegative(dst, a []int32, n int) { + for i := 0; i < n; i++ { + dst[i] = a[i-1] * 3 + } +} + +func FirstIndexZero(dst, a []int32, n int) { + for i := 1; i < n; i++ { + dst[i] = a[i-1] * 3 + } +} + +const s0, lim = 0, 16 + +func NamedLimit(dst, a []int32, k int) { + for i := k; i < lim; i++ { + dst[i] = a[i-20] + 1 + } +} + +func NamedStart(dst, a []int32, n int) { + for i := s0; i < n; i++ { + dst[i] = a[i-1] * 3 + } +} + +func ExpressionStart(dst, a []int32, n int) { + for i := 1 - 1; i < n; i++ { + dst[i] = a[i-1] * 3 + } +} + +func NamedStartCopy(dst, a []int32, n int) { + for i := s0; i < n; i++ { + dst[i] = a[i-1] + } +} +-- stdout.want -- +{"file":"$WORK/ops.go","line":4,"func":"FirstIndexZeroConstant","vectorized":true} +{"file":"$WORK/ops.go","line":10,"func":"EveryIndexNegative","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":16,"func":"FarBelow","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":22,"func":"FirstIndexNegative","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":28,"func":"FirstIndexZero","vectorized":true} +{"file":"$WORK/ops.go","line":36,"func":"NamedLimit","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":42,"func":"NamedStart","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":48,"func":"ExpressionStart","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":54,"func":"NamedStartCopy","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +-- stderr.want -- +loopvec: rewrote 2 loop(s) in $WORK/ops.go From 7678112f1691e89f685f280ecb89eff276ec21ea Mon Sep 17 00:00:00 2001 From: Mauri de Souza Meneguzzo Date: Tue, 3 Nov 2026 18:18:46 -0300 Subject: [PATCH 02/10] testdata: add a PGO regression test for the toolexec wrapper (#12) `toolexec_reproducible.txt` checks that two separate builds of a rewritten package give the same archive. It doesn't cover the failure that motivated #11. With PGO, `go test` compiles a package once for a main package's binary and again for its test binary. It then compiles the test against one copy and links the other, and the linker reports a fingerprint mismatch when the two differ. `toolexec_pgo.txt` reproduces that. A main package with a `default.pgo` has a test that imports the rewritten package, so the test binary gets a PGO copy of its own. The profile is written by a small program inside the script, because a CPU profile with no samples is enough to turn PGO on. Without #11, `go test ./...` fails with the same link error seen in goav1: ``` link: fingerprint mismatch: example.com/app/vec has d0f77d59987fdb10, import from example.com/app/cmd/bench expecting 205082f863ae44a6 ``` `toolexec_reproducible.txt` is also brought in line with the other toolexec scripts: - it checks the wrapper's log against a `log.want` section; - it checks that both builds print nothing; - it limits the log to the package under test with `LOOPVEC_TOOLEXEC_LOG_PKGS`. Tested locally: - With #11's `compile.go` reverted, both scripts fail. The PGO one fails with the link error above, and the reproducible one at `cmp a1.a a2.a`. - With #11 in place, both pass, with the same result on three repeated runs. - `make test` and `make test-qemu-arm64` pass. - `-update` leaves both scripts unchanged. --- testdata/toolexec_pgo.txt | 103 +++++++++++++++++++++++++++++ testdata/toolexec_reproducible.txt | 10 ++- 2 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 testdata/toolexec_pgo.txt diff --git a/testdata/toolexec_pgo.txt b/testdata/toolexec_pgo.txt new file mode 100644 index 0000000..974d0a6 --- /dev/null +++ b/testdata/toolexec_pgo.txt @@ -0,0 +1,103 @@ +# With PGO, go test compiles a package once for a main package's binary and +# again for its test binary, with the same inputs, and can compile the test +# against one copy and link the other. The linker rejects the build unless both +# copies of a rewritten package are the same archive. A profile with no samples +# is enough to turn PGO on. + +cd mkprof +exec go run . $WORK/cmd/bench/default.pgo +! stdout . +! stderr . +cd $WORK + +env GOEXPERIMENT=simd +env GOCACHE=$WORK/gocache +env LOOPVEC_TOOLEXEC_LOG=$WORK/log +env LOOPVEC_TOOLEXEC_LOG_PKGS=example.com/app/vec +exec go test -toolexec=$LOOPVEC_TOOLEXEC ./... +stdout -count=1 '^ok example\.com/app/vec ' +stdout -count=1 '^ok example\.com/app/cmd/bench ' +! stderr . +cmp log log.want + +-- go.mod -- +module example.com/app + +go 1.27.1 +-- vec/vec.go -- +package vec + +func Add(dst, a, b []float32) { + for i := range dst { + dst[i] = a[i] + b[i] + } +} +-- vec/vec_test.go -- +package vec + +import "testing" + +func TestAdd(t *testing.T) { + dst := make([]float32, 9) + Add(dst, []float32{1, 2, 3, 4, 5, 6, 7, 8, 9}, []float32{1, 1, 1, 1, 1, 1, 1, 1, 1}) + if dst[8] != 10 { + t.Fatal(dst) + } +} +-- cmd/bench/main.go -- +package main + +import "example.com/app/vec" + +func main() { + dst := make([]float32, 4) + vec.Add(dst, dst, dst) +} +-- cmd/bench/main_test.go -- +package main + +import ( + "testing" + + "example.com/app/vec" +) + +// The test imports vec itself, so the test binary gets a PGO copy of vec of its +// own. +func TestAdd(t *testing.T) { + dst := make([]float32, 4) + vec.Add(dst, []float32{1, 2, 3, 4}, []float32{1, 1, 1, 1}) + if dst[3] != 5 { + t.Fatal(dst) + } +} +-- mkprof/go.mod -- +module mkprof + +go 1.27.1 +-- mkprof/main.go -- +// mkprof writes a CPU profile with no samples to the file named by its argument. +package main + +import ( + "os" + "runtime/pprof" +) + +func main() { + f, err := os.Create(os.Args[1]) + if err != nil { + panic(err) + } + if err := pprof.StartCPUProfile(f); err != nil { + panic(err) + } + pprof.StopCPUProfile() + if err := f.Close(); err != nil { + panic(err) + } +} +-- log.want -- +rewrote 1 loop(s) in example.com/app/vec +rewrote 1 loop(s) in example.com/app/vec +rewrote 1 loop(s) in example.com/app/vec diff --git a/testdata/toolexec_reproducible.txt b/testdata/toolexec_reproducible.txt index ad9e082..6b4d65d 100644 --- a/testdata/toolexec_reproducible.txt +++ b/testdata/toolexec_reproducible.txt @@ -7,11 +7,16 @@ env GOEXPERIMENT=simd env LOOPVEC_TOOLEXEC_LOG=$WORK/log +env LOOPVEC_TOOLEXEC_LOG_PKGS=example.com/lib env GOCACHE=$WORK/gocache1 exec go build -buildmode=archive -toolexec=$LOOPVEC_TOOLEXEC -o a1.a . +! stdout . +! stderr . env GOCACHE=$WORK/gocache2 exec go build -buildmode=archive -toolexec=$LOOPVEC_TOOLEXEC -o a2.a . -grep -count=2 '^rewrote 1 loop\(s\) in example\.com/lib$' log +! stdout . +! stderr . +cmp log log.want cmp a1.a a2.a -- go.mod -- @@ -26,3 +31,6 @@ func Add(dst, a, b []float32) { dst[i] = a[i] + b[i] } } +-- log.want -- +rewrote 1 loop(s) in example.com/lib +rewrote 1 loop(s) in example.com/lib From 4b42aa15f7b2381395f9d5674d8ec966d82854d9 Mon Sep 17 00:00:00 2001 From: Mauri de Souza Meneguzzo Date: Tue, 3 Nov 2026 18:23:21 -0300 Subject: [PATCH 03/10] internal/loopir: keep a named constant bound's name in emitted code (#13) 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). --- AGENTS.md | 22 +- internal/loopir/ir.go | 6 + internal/loopir/lower.go | 139 +++++++---- internal/loopir/plan.go | 35 +-- internal/loopir/reason.go | 17 +- testdata/bound_arch_constant.txt | 236 ++++++++++++++++++ testdata/offset_negative_constant.txt | 221 ++++++++++++---- .../offset_negative_constant_no_rewrite.txt | 109 ++++++++ 8 files changed, 650 insertions(+), 135 deletions(-) create mode 100644 testdata/bound_arch_constant.txt create mode 100644 testdata/offset_negative_constant_no_rewrite.txt diff --git a/AGENTS.md b/AGENTS.md index cbf99b5..f0c876e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,10 +89,14 @@ safety argument stays structural: down, need one added to a bound, so they are accepted only where that cannot overflow (`plusOne`): a constant, or something minus a positive constant. A constant limit of zero or less, or a negative constant start, would put a negative - constant index in the emitted code and is rejected. `Trip` states every loop as - the forward `Start <= i < Limit`; a limit that is not exactly `len` of the stored - slice makes every operand slice carry an explicit cap (`Plan.Bound`), or the last - vector would write past it. + constant index in the emitted code and is rejected. A constant written with + literals alone (`16`, `1 - 1`) is spelled as its value; one that names a constant + keeps the name (`spell`), since the emitted file is compiled for every build and + the name can stand for another value in another (a file for each GOARCH). Checks + that need a constant read `Trip.StartValue`/`LimitValue`, never the text. `Trip` + states every loop as the forward `Start <= i < Limit`; a limit that is not exactly + `len` of the stored slice makes every operand slice carry an explicit cap + (`Plan.Bound`), or the last vector would write past it. - A slice operand is a variable or a chain of field selections ending in a slice (`x.f`, `x.f.g`) whose root variable the body does not assign (`sliceRef`). The body has no call and stores only elements, so nothing in it changes the header a field holds. The @@ -107,9 +111,13 @@ safety argument stays structural: iteration writes, so the scalar loop sees the old value and the emitter loads it before any store of the chunk (`statements` hoists those loads). A negative offset is a recurrence (`ReasonCarriedDependence`); a variable distance or a - loop that counts down is `ReasonOffsetOfStored`. The length check is - against the last index read (`lastIndex`), and offset loads are not capped at the - limit. Every other store and load is `ident[i]` (`index`) — `dst[i+1]`, `dst[2*i]` are rejected outright, regardless of + loop that counts down is `ReasonOffsetOfStored`. A constant offset that puts an + index below zero, the lowest one read with a constant start or every one with a + constant limit, is `ReasonNegativeIndex`: the scalar loop panics and the emitted + code would index at a negative constant. A positive one that pushes a constant + start or limit past the largest int is rejected as that bound, since the emitted + code adds them. The length check is against the last index read (`lastIndex`), + and offset loads are not capped at the limit. Every other store and load is `ident[i]` (`index`) — `dst[i+1]`, `dst[2*i]` are rejected outright, regardless of whether they're actually dependent. That is what guarantees no two iterations touch the same element. - A non-slice operand becomes an `Invariant` only if it is a variable, a diff --git a/internal/loopir/ir.go b/internal/loopir/ir.go index 981fe77..9b596b0 100644 --- a/internal/loopir/ir.go +++ b/internal/loopir/ir.go @@ -136,6 +136,12 @@ type Trip struct { // Start and Limit are loop-invariant integer expressions, as source text. // Start is "0" for the common case. Start, Limit string + // StartConst and LimitConst are set when Start and Limit are constants, whose + // values are StartValue and LimitValue. The text of a constant that names + // another constant keeps the name (see spell), so the checks that need a + // constant read these. + StartConst, LimitConst bool + StartValue, LimitValue int64 // Slice is the slice whose length is Limit, when Limit is exactly len(Slice). Slice *Ref // NonEmpty is set when both are constants and Start < Limit, so the loop runs diff --git a/internal/loopir/lower.go b/internal/loopir/lower.go index 8d6eb6e..dc84301 100644 --- a/internal/loopir/lower.go +++ b/internal/loopir/lower.go @@ -100,7 +100,7 @@ func (l *lowerer) rangeLoop(s *ast.RangeStmt) (*Loop, Reason) { l.valueVar = l.info.Defs[valueIdent] l.valueRef = ref } - return l.body(s, s.Body, Trip{Start: "0", Limit: "len(" + ref.Text() + ")", Slice: ref}, 1) + return l.body(s, s.Body, Trip{Start: "0", StartConst: true, Limit: "len(" + ref.Text() + ")", Slice: ref}, 1) } if s.Value != nil { return nil, ReasonUnsupportedRange @@ -215,7 +215,7 @@ func (l *lowerer) reverse(s *ast.ForStmt, start, limit ast.Expr, strict bool) (* if !isConst || low == math.MaxInt64 { return nil, ReasonUnsupportedBound } - from = bound{text: strconv.FormatInt(low+1, 10), value: low + 1, isConst: true} + from = bound{text: spell(limit, low, 1), value: low + 1, isConst: true} } else if from, ok = l.bound(limit); !ok { return nil, ReasonUnsupportedBound } @@ -269,10 +269,14 @@ type bound struct { // trip states a loop that runs from to to (exclusive) as a Trip. func (l *lowerer) trip(from, to bound) Trip { return Trip{ - Start: from.text, - Limit: to.text, - Slice: to.slice, - NonEmpty: from.isConst && to.isConst && from.value < to.value, + Start: from.text, + Limit: to.text, + StartConst: from.isConst, + LimitConst: to.isConst, + StartValue: from.value, + LimitValue: to.value, + Slice: to.slice, + NonEmpty: from.isConst && to.isConst && from.value < to.value, } } @@ -287,10 +291,7 @@ func (l *lowerer) bound(e ast.Expr) (bound, bool) { b := bound{text: types.ExprString(e)} b.value, b.isConst = l.constant(e) if b.isConst { - // A named constant or a constant expression (s0, 1 - 1) is spelled as its - // value, so the checks that read a constant start or limit see it and the - // emitted code never indexes at a negative constant they would have caught. - b.text = strconv.FormatInt(b.value, 10) + b.text = spell(e, b.value, 0) } if call, ok := ast.Unparen(e).(*ast.CallExpr); ok { b.slice, _ = l.lenOf(call) @@ -302,29 +303,57 @@ func (l *lowerer) bound(e ast.Expr) (bound, bool) { // need. It is only accepted where adding one cannot overflow: a constant below the // largest int, or something minus a positive constant. func (l *lowerer) plusOne(e ast.Expr) (bound, bool) { - if v, isConst := l.constant(e); isConst { - if v == math.MaxInt64 { - return bound{}, false - } - return bound{text: strconv.FormatInt(v+1, 10), value: v + 1, isConst: true}, true - } - sub, ok := ast.Unparen(e).(*ast.BinaryExpr) - if !ok || sub.Op != token.SUB { - return bound{}, false - } - k, isConst := l.constant(sub.Y) - if !isConst || k < 1 || !l.pureInt(sub.X) { + v, isConst := l.constant(e) + if isConst && v == math.MaxInt64 { return bound{}, false } - // x - k + 1 is x - (k-1); with k == 1 that is x itself. - x, ok := l.bound(sub.X) - if !ok { - return bound{}, false + // x - k + 1 is x - (k-1); with k == 1 that is x itself. A constant that names + // another is written this way too, so width - 1 + 1 is width. One written with + // literals alone is spelled as its value. + if sub, ok := ast.Unparen(e).(*ast.BinaryExpr); ok && sub.Op == token.SUB && !literal(e) { + if k, kConst := l.constant(sub.Y); kConst && k >= 1 { + if x, ok := l.bound(sub.X); ok { + if k == 1 { + return x, true + } + return bound{text: x.text + " - " + strconv.FormatInt(k-1, 10), value: v + 1, isConst: isConst}, true + } + } } - if k == 1 { - return x, true + if isConst { + return bound{text: spell(e, v, 1), value: v + 1, isConst: true}, true } - return bound{text: x.text + " - " + strconv.FormatInt(k-1, 10)}, true + return bound{}, false +} + +// spell is the source text of the constant e, whose value is value, plus delta. A +// constant written with literals alone (16, 1 - 1) is spelled as its value. One +// that names a constant keeps the name, since the emitted file is compiled for +// every build and the name can stand for another value in another one (a file for +// each GOARCH). +func spell(e ast.Expr, value, delta int64) string { + if literal(e) { + return strconv.FormatInt(value+delta, 10) + } + // Every integer operator binds at least as tightly as +, and operators that bind + // as tightly group from the left, so the sum needs no parentheses. + text := types.ExprString(e) + if delta != 0 { + text += " + " + strconv.FormatInt(delta, 10) + } + return text +} + +// literal reports whether e is written with literals and operators alone, no names. +func literal(e ast.Expr) bool { + found := false + ast.Inspect(e, func(n ast.Node) bool { + if _, ok := n.(*ast.Ident); ok { + found = true + } + return !found + }) + return !found } // constant returns the value of e when it is an integer constant that fits an int64. @@ -516,6 +545,17 @@ func (l *lowerer) body(node ast.Stmt, block *ast.BlockStmt, trip Trip, step int6 // the chunk. A read of an earlier iteration's element (d < 0) is a recurrence, // and a distance that is not a constant, or a loop that counts down, has no // order loopvec can check. + // + // A constant offset that puts an index below zero makes the scalar loop panic + // when it runs. Start is the lowest index the loop visits, whichever way it + // counts, so a constant start puts the lowest index read at Start+off, and a + // constant limit puts the highest at Limit-1+off. Such a loop is left alone, + // since the vector loop would index or slice at that negative constant, which + // does not compile. Start is never negative and Limit never below one (see + // rangeReason), so only a negative offset can do this, and it is at least + // -maxOffset, so negating it cannot overflow. A positive offset that pushes a + // constant start or limit past the largest int would not compile either, as the + // emitted code adds them. stored := map[types.Object]bool{} for _, stmt := range body { if store, ok := stmt.(Store); ok { @@ -525,35 +565,30 @@ func (l *lowerer) body(node ast.Stmt, block *ast.BlockStmt, trip Trip, step int6 for _, stmt := range body { for _, leaf := range Leaves(stmtValue(stmt)) { load, ok := leaf.(*Load) - if !ok || load.Off == "" || !stored[load.Ref.Obj] { + if !ok || load.Off == "" { continue } distance, err := strconv.ParseInt(load.Off, 10, 64) - switch { - case err != nil || step != 1: - return nil, ReasonOffsetOfStored - case distance < 0: - return nil, ReasonCarriedDependence + if stored[load.Ref.Obj] { + switch { + case err != nil || step != 1: + return nil, ReasonOffsetOfStored + case distance < 0: + return nil, ReasonCarriedDependence + } } - } - } - - // A constant offset that puts an index below zero makes the scalar loop panic - // when it runs: on its first iteration when the start is a constant and the - // first index is negative, or on every iteration when the limit is a constant - // and even the last index is. It is left alone, since the vector loop would - // index or slice at a negative constant, which does not compile. - start, errStart := strconv.ParseInt(trip.Start, 10, 64) - limit, errLimit := strconv.ParseInt(trip.Limit, 10, 64) - negative := func(offset string) bool { - off, err := strconv.ParseInt(offset, 10, 64) - return err == nil && (errStart == nil && start+off < 0 || errLimit == nil && limit+off <= 0) - } - for _, stmt := range body { - for _, leaf := range Leaves(stmtValue(stmt)) { - if load, ok := leaf.(*Load); ok && negative(load.Off) { + if err == nil && distance < 0 && + (trip.StartConst && trip.StartValue < -distance || trip.LimitConst && trip.LimitValue <= -distance) { return nil, ReasonNegativeIndex } + if err == nil && distance > 0 { + switch { + case trip.StartConst && trip.StartValue > math.MaxInt64-distance: + return nil, ReasonUnsupportedStart + case trip.LimitConst && trip.LimitValue > math.MaxInt64-distance: + return nil, ReasonUnsupportedBound + } + } } } diff --git a/internal/loopir/plan.go b/internal/loopir/plan.go index d4bf63b..bbe9e61 100644 --- a/internal/loopir/plan.go +++ b/internal/loopir/plan.go @@ -1,9 +1,6 @@ package loopir -import ( - "go/types" - "strconv" -) +import "go/types" // maxSlices is the most distinct slices a loop may touch. Each stored slice is // checked at run time against every other slice in the loop, so the number of @@ -166,22 +163,18 @@ func NewPlan(l *Loop, opts Options) (*Plan, Reason) { p.Stmts = append(p.Stmts, planned) } if len(p.Stmts) == 1 && p.Stmts[0].Dst != nil { - if load, ok := p.Stmts[0].Root.(*Load); ok { - p.Copy = !negativeSource(l.Ind.Trip.Start, load.Off) - } + _, p.Copy = p.Stmts[0].Root.(*Load) } trip := l.Ind.Trip // A loop that runs a few times, known at compile time, is faster as it is. - if from, errFrom := strconv.ParseInt(trip.Start, 10, 64); errFrom == nil { - if to, errTo := strconv.ParseInt(trip.Limit, 10, 64); errTo == nil { - least := int64(minTrip) - if p.Full { - least = int64(MinReduceVectors * MaxLanes(elem)) - } - if to-from < least { - return nil, ReasonShortLoop - } + if trip.StartConst && trip.LimitConst { + least := int64(minTrip) + if p.Full { + least = int64(MinReduceVectors * MaxLanes(elem)) + } + if trip.LimitValue-trip.StartValue < least { + return nil, ReasonShortLoop } } p.Start, p.Limit, p.NonEmpty, p.TripRef = trip.Start, trip.Limit, trip.NonEmpty, trip.Slice @@ -228,16 +221,6 @@ func NewPlan(l *Loop, opts Options) (*Plan, Reason) { return p, "" } -// negativeSource reports whether a copy would slice its source at a negative -// constant, which does not compile: the loop starts at start and reads at offset -// off, and both are constants that add up to less than zero. Such a loop panics -// when it runs, which the ordinary vector loop does as well. -func negativeSource(start, off string) bool { - s, errStart := strconv.ParseInt(start, 10, 64) - o, errOff := strconv.ParseInt(off, 10, 64) - return errStart == nil && errOff == nil && s+o < 0 -} - // checkOps returns why simd cannot express an operation in v on the element // type, or "". func checkOps(elem types.Type, v Value) Reason { diff --git a/internal/loopir/reason.go b/internal/loopir/reason.go index 4757287..50e049c 100644 --- a/internal/loopir/reason.go +++ b/internal/loopir/reason.go @@ -17,7 +17,9 @@ const ( ReasonUnsupportedRange Reason = "range clause is not i := range s, i, v := range s, or i := range n" // ReasonUnsupportedStart covers a counting loop whose start is not a // loop-invariant integer expression (a constant, an int variable, len of a - // slice, and sums and products of those), or is a negative constant. + // slice, and sums and products of those), is a negative constant, or is a + // constant that a read at a positive constant offset pushes past the largest + // int. ReasonUnsupportedStart Reason = "loop start is not a loop-invariant integer expression" // ReasonUnsupportedCondition covers a loop condition that is not // i < limit or i <= limit, or i >= limit or i > limit when counting down. @@ -28,7 +30,9 @@ const ( // ReasonUnsupportedBound covers a limit that is not a loop-invariant integer // expression, is a constant that is not positive, or is written so that // adding one to it could overflow (a <= limit, or the start of a loop that - // counts down, that is not a constant or something minus a constant). + // counts down, that is not a constant or something minus a constant), or is + // a constant that a read at a positive constant offset pushes past the + // largest int. ReasonUnsupportedBound Reason = "loop limit is not a loop-invariant integer expression" // ReasonUnsupportedDestination covers a destination that isn't a plain // slice[i] element: an offset (dst[i+1]), a stride (dst[2*i]), or a @@ -43,10 +47,11 @@ const ( // wrote, so the iterations cannot run as a vector. ReasonCarriedDependence Reason = "an iteration reads an element an earlier iteration wrote" // ReasonNegativeIndex covers a slice read at a constant offset that puts an - // index below zero: the first index, with a constant start (a[i-1] in a loop - // from 0), or every index, with a constant limit (a[i-20] in a loop that stops - // before 16). The scalar loop panics when it runs, and the vector loop would - // index or slice at a negative constant, which does not compile. + // index below zero: the one read at the lowest index the loop visits, with a + // constant start (a[i-1] in a loop over 0..n-1, whichever way it counts), or + // every one, with a constant limit (a[i-20] in a loop that stops before 16). The + // scalar loop panics when it reaches that index, and the vector loop would index + // or slice at a negative constant, which does not compile. ReasonNegativeIndex Reason = "a constant offset puts an index below zero" // ReasonUnsupportedIf covers an if that is not a select: a condition that is // not a comparison of loop values (joined by && and ||), an init statement, or diff --git a/testdata/bound_arch_constant.txt b/testdata/bound_arch_constant.txt new file mode 100644 index 0000000..649af02 --- /dev/null +++ b/testdata/bound_arch_constant.txt @@ -0,0 +1,236 @@ +# A start or limit that names a constant keeps the name in the emitted code, since +# the file is compiled for every build and the constant can have another value in +# another one. Here width differs by GOARCH: written out as its value on one +# architecture, the vector loop would stop short, or run past the slices, on the +# others. A constant written with literals alone is spelled as its value. + +exec go run ./testmain +cmp stdout testmain/output + +loopvec -split . +! stdout . +cmpenv stderr stderr.want +cmp ops_simd.go ops_simd.go.want + +env GOEXPERIMENT=simd +exec go run ./testmain +cmp stdout testmain/output + +-- go.mod -- +module example.com/test + +go 1.27.1 +-- width_amd64.go -- +package test + +const width = 32 +-- width_arm64.go -- +package test + +const width = 64 +-- width_other.go -- +//go:build !amd64 && !arm64 + +package test + +const width = 48 +-- width.go -- +package test + +// Width is width, for testmain to check against. +func Width() int { return width } +-- ops.go -- +package test + +func Add(dst, a, b []float32) { + for i := 0; i < width; i++ { + dst[i] = a[i] + b[i] + } +} + +func AddInclusive(dst, a, b []float32) { + for i := 0; i <= width-1; i++ { + dst[i] = a[i] + b[i] + } +} + +func AddDown(dst, a, b []float32) { + for i := width - 1; i >= 0; i-- { + dst[i] = a[i] + b[i] + } +} + +func AddAbove(dst, a, b []float32) { + for i := len(dst) - 1; i > width; i-- { + dst[i] = a[i] + b[i] + } +} + +func AddLiteral(dst, a, b []float32) { + for i := 0; i < 2*8; i++ { + dst[i] = a[i] + b[i] + } +} +-- ops_simd.go.want -- +//go:build goexperiment.simd + +package test + +import ( + "simd" + "unsafe" +) + +func Add(dst, a, b []float32) { + if _loopvecOverlap(dst, a, 0, width, 0, width) || _loopvecOverlap(dst, b, 0, width, 0, width) { + for i := 0; i < width; i++ { + dst[i] = a[i] + b[i] + } + } else { + _ = dst[width-1] + _ = a[width-1] + _ = b[width-1] + for _i := 0; _i < width; { + _v1, _n := simd.LoadFloat32sPart(a[_i:width]) + _v2, _ := simd.LoadFloat32sPart(b[_i:width]) + _v1.Add(_v2).StorePart(dst[_i:width]) + _i += _n + } + } +} + +func AddInclusive(dst, a, b []float32) { + if _loopvecOverlap(dst, a, 0, width, 0, width) || _loopvecOverlap(dst, b, 0, width, 0, width) { + for i := 0; i <= width-1; i++ { + dst[i] = a[i] + b[i] + } + } else { + _ = dst[width-1] + _ = a[width-1] + _ = b[width-1] + for _i := 0; _i < width; { + _v1, _n := simd.LoadFloat32sPart(a[_i:width]) + _v2, _ := simd.LoadFloat32sPart(b[_i:width]) + _v1.Add(_v2).StorePart(dst[_i:width]) + _i += _n + } + } +} + +func AddDown(dst, a, b []float32) { + if _loopvecOverlap(dst, a, 0, width, 0, width) || _loopvecOverlap(dst, b, 0, width, 0, width) { + for i := width - 1; i >= 0; i-- { + dst[i] = a[i] + b[i] + } + } else { + _ = dst[width-1] + _ = a[width-1] + _ = b[width-1] + for _i := 0; _i < width; { + _v1, _n := simd.LoadFloat32sPart(a[_i:width]) + _v2, _ := simd.LoadFloat32sPart(b[_i:width]) + _v1.Add(_v2).StorePart(dst[_i:width]) + _i += _n + } + } +} + +func AddAbove(dst, a, b []float32) { + if _loopvecOverlap(dst, a, width+1, len(dst), width+1, len(dst)) || _loopvecOverlap(dst, b, width+1, len(dst), width+1, len(dst)) { + for i := len(dst) - 1; i > width; i-- { + dst[i] = a[i] + b[i] + } + } else { + if len(dst) > width+1 { + _ = a[len(dst)-1] + _ = b[len(dst)-1] + for _i := width + 1; _i < len(dst); { + _v1, _n := simd.LoadFloat32sPart(a[_i:]) + _v2, _ := simd.LoadFloat32sPart(b[_i:]) + _v1.Add(_v2).StorePart(dst[_i:]) + _i += _n + } + } + } +} + +func AddLiteral(dst, a, b []float32) { + if _loopvecOverlap(dst, a, 0, 16, 0, 16) || _loopvecOverlap(dst, b, 0, 16, 0, 16) { + for i := 0; i < 2*8; i++ { + dst[i] = a[i] + b[i] + } + } else { + _ = dst[16-1] + _ = a[16-1] + _ = b[16-1] + for _i := 0; _i < 16; { + _v1, _n := simd.LoadFloat32sPart(a[_i:16]) + _v2, _ := simd.LoadFloat32sPart(b[_i:16]) + _v1.Add(_v2).StorePart(dst[_i:16]) + _i += _n + } + } +} + +func _loopvecOverlap[T any](a, b []T, window ...int) bool { + aLo, aHi, bLo, bHi := 0, len(a), 0, len(b) + if len(window) == 4 { + aLo, aHi, bLo, bHi = window[0], window[1], window[2], window[3] + } + if aLo >= aHi || bLo >= bHi { + return false + } + size := unsafe.Sizeof(a[0]) + aBase := uintptr(unsafe.Pointer(unsafe.SliceData(a))) + bBase := uintptr(unsafe.Pointer(unsafe.SliceData(b))) + aStart, aEnd := aBase+uintptr(aLo)*size, aBase+uintptr(aHi)*size + bStart, bEnd := bBase+uintptr(bLo)*size, bBase+uintptr(bHi)*size + return aStart < bEnd && bStart < aEnd +} +-- stderr.want -- +loopvec: rewrote 5 loop(s) in $WORK/ops.go +-- testmain/main.go -- +package main + +import ( + "fmt" + + "example.com/test" +) + +const n = 100 + +// check reports whether f stored a[i] + b[i] at exactly the indexes lo..hi-1 of +// dst and left every other element alone. +func check(f func(dst, a, b []float32), lo, hi int) bool { + dst, a, b := make([]float32, n), make([]float32, n), make([]float32, n) + for i := range a { + a[i], b[i], dst[i] = float32(i), float32(2*i), -1 + } + f(dst, a, b) + for i := range dst { + want := float32(-1) + if lo <= i && i < hi { + want = a[i] + b[i] + } + if dst[i] != want { + return false + } + } + return true +} + +func main() { + w := test.Width() + fmt.Println(check(test.Add, 0, w)) + fmt.Println(check(test.AddInclusive, 0, w)) + fmt.Println(check(test.AddDown, 0, w)) + fmt.Println(check(test.AddAbove, w+1, n)) + fmt.Println(check(test.AddLiteral, 0, 16)) +} +-- testmain/output -- +true +true +true +true +true diff --git a/testdata/offset_negative_constant.txt b/testdata/offset_negative_constant.txt index d23cc26..6927e61 100644 --- a/testdata/offset_negative_constant.txt +++ b/testdata/offset_negative_constant.txt @@ -1,18 +1,20 @@ # A read at a negative constant offset is rewritten while every index it reads can -# be zero or more. When an index is a negative constant, the first one in a loop -# with a constant start (a[i-1] from 0) or every one in a loop with a constant limit -# (a[i-16] in a loop that stops before 16), the scalar loop panics when it runs and -# the vector loop would index at a negative constant, which does not compile, so the -# loop is left alone, whether the start and limit are written as literals, named -# constants or constant expressions. The rewritten package must build. +# be zero or more: from a constant start at least as large as the offset, a named +# one included, which the emitted code keeps by name, and up to a constant limit +# that leaves the highest index read at zero or more. Loops whose index is a +# negative constant are left alone (see offset_negative_constant_no_rewrite.txt). -loopvec -json . -cmpenv stdout stdout.want +exec go run ./testmain +cmp stdout testmain/output loopvec -split . +! stdout . cmpenv stderr stderr.want +cmp ops_simd.go ops_simd.go.want + env GOEXPERIMENT=simd -exec go build . +exec go run ./testmain +cmp stdout testmain/output -- go.mod -- module example.com/test @@ -27,64 +29,195 @@ func FirstIndexZeroConstant(dst, a []int32) { } } -func EveryIndexNegative(dst, a []int32) { - for i := 0; i < 16; i++ { - dst[i] = a[i-16] + 1 +func FirstIndexZero(dst, a []int32, n int) { + for i := 1; i < n; i++ { + dst[i] = a[i-1] * 3 } } -func FarBelow(dst, a []float32) { - for i := 0; i < 16; i++ { - dst[i] = a[i-20] * a[i] +const s1 = 1 + +func NamedStart(dst, a []int32, n int) { + for i := s1; i < n; i++ { + dst[i] = a[i-1] * 3 } } -func FirstIndexNegative(dst, a []int32, n int) { - for i := 0; i < n; i++ { - dst[i] = a[i-1] * 3 +func NamedStartCopy(dst, a []int32, n int) { + for i := s1; i < n; i++ { + dst[i] = a[i-1] } } -func FirstIndexZero(dst, a []int32, n int) { - for i := 1; i < n; i++ { - dst[i] = a[i-1] * 3 +func LastIndexZero(dst, a []int32, k int) { + for i := k; i < 16; i++ { + dst[i] = a[i-15] + 1 } } +-- ops_simd.go.want -- +//go:build goexperiment.simd -const s0, lim = 0, 16 +package test + +import ( + "simd" + "unsafe" +) -func NamedLimit(dst, a []int32, k int) { - for i := k; i < lim; i++ { - dst[i] = a[i-20] + 1 +func FirstIndexZeroConstant(dst, a []int32) { + _vcInt32s0 := simd.BroadcastInt32s(1) + if _loopvecOverlap(dst, a, 15, 31, 0, 31) { + for i := 15; i < 31; i++ { + dst[i] = a[i-15] + 1 + } + } else { + _ = dst[31-1] + _ = a[31-16] + for _i := 15; _i < 31; { + _v1, _n := simd.LoadInt32sPart(a[_i-15:]) + _v1.Add(_vcInt32s0).StorePart(dst[_i:31]) + _i += _n + } } } -func NamedStart(dst, a []int32, n int) { - for i := s0; i < n; i++ { - dst[i] = a[i-1] * 3 +func FirstIndexZero(dst, a []int32, n int) { + _vcInt32s1 := simd.BroadcastInt32s(3) + if _loopvecOverlap(dst, a, 1, n, 0, n) { + for i := 1; i < n; i++ { + dst[i] = a[i-1] * 3 + } + } else { + if n > 1 { + _ = dst[n-1] + _ = a[n-2] + for _i := 1; _i < n; { + _v1, _n := simd.LoadInt32sPart(a[_i-1:]) + _v1.Mul(_vcInt32s1).StorePart(dst[_i:n]) + _i += _n + } + } } } -func ExpressionStart(dst, a []int32, n int) { - for i := 1 - 1; i < n; i++ { - dst[i] = a[i-1] * 3 +const s1 = 1 + +func NamedStart(dst, a []int32, n int) { + _vcInt32s2 := simd.BroadcastInt32s(3) + if _loopvecOverlap(dst, a, s1, n, s1-1, n) { + for i := s1; i < n; i++ { + dst[i] = a[i-1] * 3 + } + } else { + if n > s1 { + _ = dst[n-1] + _ = a[n-2] + for _i := s1; _i < n; { + _v1, _n := simd.LoadInt32sPart(a[_i-1:]) + _v1.Mul(_vcInt32s2).StorePart(dst[_i:n]) + _i += _n + } + } } } func NamedStartCopy(dst, a []int32, n int) { - for i := s0; i < n; i++ { - dst[i] = a[i-1] + if _loopvecOverlap(dst, a, s1, n, s1-1, n) { + for i := s1; i < n; i++ { + dst[i] = a[i-1] + } + } else { + if n > s1 { + _ = dst[n-1] + _ = a[n-2] + copy(dst[s1:n], a[s1-1:]) + } + } +} + +func LastIndexZero(dst, a []int32, k int) { + _vcInt32s4 := simd.BroadcastInt32s(1) + if _loopvecOverlap(dst, a, k, 16, k-15, 16) { + for i := k; i < 16; i++ { + dst[i] = a[i-15] + 1 + } + } else { + if 16 > k { + _ = dst[16-1] + _ = a[16-16] + for _i := k; _i < 16; { + _v1, _n := simd.LoadInt32sPart(a[_i-15:]) + _v1.Add(_vcInt32s4).StorePart(dst[_i:16]) + _i += _n + } + } } } --- stdout.want -- -{"file":"$WORK/ops.go","line":4,"func":"FirstIndexZeroConstant","vectorized":true} -{"file":"$WORK/ops.go","line":10,"func":"EveryIndexNegative","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} -{"file":"$WORK/ops.go","line":16,"func":"FarBelow","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} -{"file":"$WORK/ops.go","line":22,"func":"FirstIndexNegative","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} -{"file":"$WORK/ops.go","line":28,"func":"FirstIndexZero","vectorized":true} -{"file":"$WORK/ops.go","line":36,"func":"NamedLimit","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} -{"file":"$WORK/ops.go","line":42,"func":"NamedStart","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} -{"file":"$WORK/ops.go","line":48,"func":"ExpressionStart","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} -{"file":"$WORK/ops.go","line":54,"func":"NamedStartCopy","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} + +func _loopvecOverlap[T any](a, b []T, window ...int) bool { + aLo, aHi, bLo, bHi := 0, len(a), 0, len(b) + if len(window) == 4 { + aLo, aHi, bLo, bHi = window[0], window[1], window[2], window[3] + } + if aLo >= aHi || bLo >= bHi { + return false + } + size := unsafe.Sizeof(a[0]) + aBase := uintptr(unsafe.Pointer(unsafe.SliceData(a))) + bBase := uintptr(unsafe.Pointer(unsafe.SliceData(b))) + aStart, aEnd := aBase+uintptr(aLo)*size, aBase+uintptr(aHi)*size + bStart, bEnd := bBase+uintptr(bLo)*size, bBase+uintptr(bHi)*size + return aStart < bEnd && bStart < aEnd +} -- stderr.want -- -loopvec: rewrote 2 loop(s) in $WORK/ops.go +loopvec: rewrote 5 loop(s) in $WORK/ops.go +-- testmain/main.go -- +package main + +import ( + "fmt" + + "example.com/test" +) + +func try(f func()) (panicked bool) { + defer func() { panicked = recover() != nil }() + f() + return +} + +func ramp(n int) []int32 { + s := make([]int32, n) + for i := range s { + s[i] = int32(i + 1) + } + return s +} + +func main() { + dst := make([]int32, 31) + test.FirstIndexZeroConstant(dst, ramp(16)) + fmt.Println(dst) + + for _, f := range []func(dst, a []int32, n int){test.FirstIndexZero, test.NamedStart, test.NamedStartCopy} { + dst := make([]int32, 21) + f(dst, ramp(21), 21) + fmt.Println(dst) + fmt.Println(try(func() { f(make([]int32, 21), ramp(19), 21) })) + } + + dst = make([]int32, 16) + test.LastIndexZero(dst, ramp(1), 15) + fmt.Println(dst) + fmt.Println(try(func() { test.LastIndexZero(make([]int32, 16), ramp(1), 14) })) +} +-- testmain/output -- +[0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17] +[0 3 6 9 12 15 18 21 24 27 30 33 36 39 42 45 48 51 54 57 60] +true +[0 3 6 9 12 15 18 21 24 27 30 33 36 39 42 45 48 51 54 57 60] +true +[0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20] +true +[0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 2] +true diff --git a/testdata/offset_negative_constant_no_rewrite.txt b/testdata/offset_negative_constant_no_rewrite.txt new file mode 100644 index 0000000..abad41e --- /dev/null +++ b/testdata/offset_negative_constant_no_rewrite.txt @@ -0,0 +1,109 @@ +# A read at a negative constant offset is left alone when an index it reads is a +# negative constant: the one at the lowest index the loop visits, with a constant +# start (a[i-1] over 0..n-1, whichever way the loop counts, and over a range of a +# slice), or every one, with a constant limit (a[i-16] in a loop that stops before +# 16). The scalar loop panics when it reaches that index, and the vector loop would +# index at a negative constant, which does not compile. The start and limit may be +# literals, named constants or constant expressions (see offset_negative_constant.txt +# for the loops that are rewritten). A read at a positive constant offset that +# pushes a constant start or limit past the largest int is left alone too, since the +# emitted code would add them. + +cp ops.go ops.go.orig +loopvec -split . +! stdout . +! stderr . +! exists ops_simd.go +cmp ops.go ops.go.orig + +loopvec -json . +cmpenv stdout stdout.want + +-- go.mod -- +module example.com/test + +go 1.27.1 +-- ops.go -- +package test + +import "math" + +func EveryIndexNegative(dst, a []int32) { + for i := 0; i < 16; i++ { + dst[i] = a[i-16] + 1 + } +} + +func FarBelow(dst, a []float32) { + for i := 0; i < 16; i++ { + dst[i] = a[i-20] * a[i] + } +} + +func FirstIndexNegative(dst, a []int32, n int) { + for i := 0; i < n; i++ { + dst[i] = a[i-1] * 3 + } +} + +const s0, lim = 0, 16 + +func NamedLimit(dst, a []int32, k int) { + for i := k; i < lim; i++ { + dst[i] = a[i-20] + 1 + } +} + +func NamedStart(dst, a []int32, n int) { + for i := s0; i < n; i++ { + dst[i] = a[i-1] * 3 + } +} + +func ExpressionStart(dst, a []int32, n int) { + for i := 1 - 1; i < n; i++ { + dst[i] = a[i-1] * 3 + } +} + +func NamedStartCopy(dst, a []int32, n int) { + for i := s0; i < n; i++ { + dst[i] = a[i-1] + } +} + +func CountingDownToZero(dst, a []int32) { + for i := 15; i >= 0; i-- { + dst[i] = a[i-1] + 1 + } +} + +func RangeOverSlice(dst, a []int32) { + for i := range dst { + dst[i] = a[i-1] + 1 + } +} + +func LimitPastLargest(dst, a []int32) { + for i := 0; i < math.MaxInt64; i++ { + dst[i] = a[i+1] + 1 + } +} + +func StartPastLargest(dst, a []int32, n int) { + for i := math.MaxInt64 - 1; i < n; i++ { + dst[i] = a[i+2] + 1 + } +} +-- stdout.want -- +{"file":"$WORK/ops.go","line":6,"func":"EveryIndexNegative","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":12,"func":"FarBelow","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":18,"func":"FirstIndexNegative","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":26,"func":"NamedLimit","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":32,"func":"NamedStart","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":38,"func":"ExpressionStart","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":44,"func":"NamedStartCopy","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":50,"func":"CountingDownToZero","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":56,"func":"RangeOverSlice","vectorized":false,"reason":"a constant offset puts an index below zero","stage":"lower"} +{"file":"$WORK/ops.go","line":62,"func":"LimitPastLargest","vectorized":false,"reason":"loop limit is not a loop-invariant integer expression","stage":"lower"} +{"file":"$WORK/ops.go","line":68,"func":"StartPastLargest","vectorized":false,"reason":"loop start is not a loop-invariant integer expression","stage":"lower"} From 8c1442980a48f5463d2e48314aab8876b50c6879 Mon Sep 17 00:00:00 2001 From: Mauri de Souza Meneguzzo Date: Tue, 3 Nov 2026 18:31:27 -0300 Subject: [PATCH 04/10] Makefile: give test-qemu-arm64 a 30m timeout (#14) In 2196e07 I added `test-qemu-arm64` without a `-timeout`, so the root package gets `go test`'s default of 10 minutes. On CI, the arm64 qemu step has taken anywhere from 237 to 438 seconds. The tip job of #12, which adds one more toolexec script, hit the limit: ``` panic: test timed out after 10m0s running tests: TestScripts/flag_precedence (2m3s) TestScripts/min_max_int (2m10s) ``` Nothing was stuck. The emulated suite is just slow, and runner speed varies. Locally under qemu, `toolexec_pgo` takes about as long as the other toolexec scripts, around 25 seconds of wall time with the scripts running in parallel. This change passes `-timeout=30m` to the target. `make test-qemu-arm64` passes locally with the change. After this merges, #12 needs a rerun, or a rebase onto master. --- Makefile | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 7b6f652..b75e67b 100644 --- a/Makefile +++ b/Makefile @@ -8,9 +8,10 @@ test: build go test ./... -count=1 -race # The scripts run the arm64 binaries they build, which needs an aarch64 binfmt -# handler as well as qemu-aarch64-static. +# handler as well as qemu-aarch64-static. Under emulation the script suite takes +# several times as long, near go test's default 10m timeout on a CI runner. test-qemu-arm64: build - GOARCH=arm64 GOOS=linux CGO_ENABLED=0 go test ./... -count=1 -exec=qemu-aarch64-static + GOARCH=arm64 GOOS=linux CGO_ENABLED=0 go test ./... -count=1 -timeout=30m -exec=qemu-aarch64-static bench-fuzz: GOEXPERIMENT=simd go test ./bench/ -count=1 -run Fuzz From 7fdec23ffa6958b02476584cc88c623860b1109c Mon Sep 17 00:00:00 2001 From: Mauri de Souza Meneguzzo Date: Tue, 3 Nov 2026 19:05:01 -0300 Subject: [PATCH 05/10] all: test against go1.27.3 (#15) This bumps the pinned toolchain from go1.27.1 to go1.27.3, the latest 1.27 release. It supersedes the go1.27.3 commit in #1. That commit bumped only the Makefile. The script harness doesn't read the Makefile: `script_test.go` runs every script with the toolchain `go.mod` names (`goModToolchain`), and `actions/setup-go` reads the same file. With only the Makefile changed, the scripts would still have run on go1.27.1, including the `flag_methods_ice.txt` check behind the claim that the crash is still in 1.27.3. This PR: - **Pins the toolchain in two places.** The Makefile now pins `go1.27.3`, and `go.mod` gets a `toolchain go1.27.3` line. - **Keeps the minimum at 1.27.1.** The `go` line stays at `1.27.1`, the first release whose simd works with what loopvec generates: go1.27.0 compiles `ShiftAllRight` with a `uint8` count. - **Regenerates `caps_gen.go`.** go1.27.3's simd package has the same methods as 1.27.1's, so only the header comment changes. - **Updates the version in `BUGS.md` and `flag_methods_ice.txt`.** The `-methods` internal compiler error (golang/go#80657) still happens on go1.27.3, and the script checks that now. Tested locally: - `make ci` passes on this commit: `test-update` leaves the goldens unchanged, and the race suite, both arm64 qemu runs, bench fuzz seeds, tsvc, fix and lint all pass. - The script harness reports `GOTOOLCHAIN=go1.27.3`, and `flag_methods_ice.txt` still gets the internal compiler error. --- BUGS.md | 6 +++--- Makefile | 2 +- go.mod | 2 ++ internal/loopir/caps_gen.go | 2 +- testdata/flag_methods_ice.txt | 2 +- 5 files changed, 8 insertions(+), 6 deletions(-) diff --git a/BUGS.md b/BUGS.md index 843087e..55231c5 100644 --- a/BUGS.md +++ b/BUGS.md @@ -2,11 +2,11 @@ loopvec rewrites code to use `GOEXPERIMENT=simd`, which is new, so most of what it hits is in the Go toolchain. This file lists the Go issues that affect loopvec. "tip" means -gotip, "1.27.1" is the release toolchain loopvec is tested against. +gotip, "1.27.3" is the release toolchain loopvec is tested against. | Issue | Status | Affects | loopvec's answer | |---|---|---|---| -| [#80657](https://github.com/golang/go/issues/80657) simd in a method: internal compiler error | fixed on tip (CL 839405), still crashes on 1.27.1 | `-methods` | methods are skipped unless `-methods` is passed | +| [#80657](https://github.com/golang/go/issues/80657) simd in a method: internal compiler error | fixed on tip (CL 839405), still crashes on 1.27.3 | `-methods` | methods are skipped unless `-methods` is passed | | [#80835](https://github.com/golang/go/issues/80835) legacy SSE encodings in functions using simd cause AVX-SSE transition penalties | open, fix pending (CLs 825145, 825146, in tip) | speed of any scalar float code that runs after a rewritten loop, in the same function or another | none; the portable `simd` package has no `ClearAVXUpperBits` | | [#81844](https://github.com/golang/go/issues/81844) blank identifier parameters rejected | fixed on tip (CL 841145) | every rewrite of a function with a `_` parameter | the generated file renames them `_p0`, `_p1`, ... | | [#81845](https://github.com/golang/go/issues/81845) calls left unresolved when inlined into a package initializer | closed as a duplicate of [#80689](https://github.com/golang/go/issues/80689), which is open | loops in package-level `var x = func() {...}()` | not rewritten (the generated initializer fails to link), see `package_initializer_no_rewrite.txt` and `toolexec_package_initializer.txt` | @@ -19,7 +19,7 @@ gotip, "1.27.1" is the release toolchain loopvec is tested against. A function with a receiver that contains simd code failed to compile with `internal compiler error: missing Types entry`. loopvec does not rewrite loops in methods -by default; `-methods` turns it on. The fix is on tip. On 1.27.1 the crash is still there, +by default; `-methods` turns it on. The fix is on tip. On 1.27.3 the crash is still there, and `flag_methods_ice.txt` pins it, so that test starts failing when the pinned toolchain gets the fix. Reported by someone else; the fix is [CL 839405](https://go.dev/cl/839405). diff --git a/Makefile b/Makefile index b75e67b..ac89073 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -export GOTOOLCHAIN ?= go1.27.1 +export GOTOOLCHAIN ?= go1.27.3 .DEFAULT_GOAL := build diff --git a/go.mod b/go.mod index 80c0f21..f1a8121 100644 --- a/go.mod +++ b/go.mod @@ -2,6 +2,8 @@ module github.com/mauri870/loopvec go 1.27.1 +toolchain go1.27.3 + require ( golang.org/x/tools v0.52.1-0.20261103172851-aba6dfd3c19d rsc.io/script v0.0.2 diff --git a/internal/loopir/caps_gen.go b/internal/loopir/caps_gen.go index bccacfa..ba7ddff 100644 --- a/internal/loopir/caps_gen.go +++ b/internal/loopir/caps_gen.go @@ -1,4 +1,4 @@ -// Code generated by capsgen from the simd package of go1.27.1. DO NOT EDIT. +// Code generated by capsgen from the simd package of go1.27.3. DO NOT EDIT. package loopir diff --git a/testdata/flag_methods_ice.txt b/testdata/flag_methods_ice.txt index 0948c91..c2ec597 100644 --- a/testdata/flag_methods_ice.txt +++ b/testdata/flag_methods_ice.txt @@ -1,4 +1,4 @@ -# -methods rewrites method bodies, but go1.27.1 cannot compile them under +# -methods rewrites method bodies, but go1.27.3 cannot compile them under # GOEXPERIMENT=simd: the compiler crashes (golang/go#80657, fixed in later # toolchains). This pins the crash so the README limitation stays accurate. # When this script starts failing on a newer toolchain, the limitation is gone: From 08b91dc394281c0c3020fd8adac20905290b5c83 Mon Sep 17 00:00:00 2001 From: Mauri de Souza Meneguzzo Date: Tue, 3 Nov 2026 19:41:43 -0300 Subject: [PATCH 06/10] loopvec: compute -d diffs in process (#16) `printDiff` wrote both sources to temporary files and ran the system `diff`. That made `-d` depend on the host. Windows has no `diff`. GNU and BSD diff printed different headers until #10 labeled them. And the error was ignored, so a missing or failing `diff` printed nothing while loopvec still exited 0. This change computes the unified diff in process with [github.com/pmezard/go-difflib](https://github.com/pmezard/go-difflib) (BSD-3, about 1,300 importing packages, mostly through testify). The labels stay the same, with both sides named after the file path and three lines of context. The temporary files and the `exec` call are gone. The library's own `SplitLines` adds an empty line after a final newline, which would show up as a context line past the end of the file, so `printDiff` splits the lines itself. go-difflib can align a hunk differently from GNU diff. For example, it may put an added line on the other side of an unchanged blank line or `}`. The patches still apply the same way. I checked this by running `loopvec -d std` with the old and new code: all 17 files that get a diff apply with `patch -F0` and give exactly the same source as GNU diff's patches. Since the output no longer depends on the machine, `flag_diff.txt` now checks it whole instead of line by line. Its patch also applies with `patch -F0`. `make ci` passes. --- go.mod | 1 + go.sum | 1 + main.go | 47 +++++++++++++------------------- testdata/flag_diff.txt | 62 ++++++++++++++++++++++++++++++++++++------ 4 files changed, 74 insertions(+), 37 deletions(-) diff --git a/go.mod b/go.mod index f1a8121..2025be4 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,7 @@ go 1.27.1 toolchain go1.27.3 require ( + github.com/pmezard/go-difflib v1.0.0 golang.org/x/tools v0.52.1-0.20261103172851-aba6dfd3c19d rsc.io/script v0.0.2 ) diff --git a/go.sum b/go.sum index 457ea15..73d56f3 100644 --- a/go.sum +++ b/go.sum @@ -478,6 +478,7 @@ github.com/pelletier/go-toml/v2 v2.4.3/go.mod h1:2gIqNv+qfxSVS7cM2xJQKtLSTLUE9V8 github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/prometheus/client_golang v0.9.1/go.mod h1:7SWBe2y4D6OKWSNQJUaRYU/AaXPKyh/dDVn+NZz0KFw= github.com/prometheus/client_golang v1.0.0/go.mod h1:db9x61etRT2tGnBNRi70OPL5FsnadC4Ky3P0J6CfImo= diff --git a/main.go b/main.go index 61dd22f..adb0b26 100644 --- a/main.go +++ b/main.go @@ -21,11 +21,11 @@ import ( "go/format" "go/token" "os" - "os/exec" "runtime" "runtime/pprof" "strings" + "github.com/pmezard/go-difflib/difflib" "golang.org/x/tools/go/packages" "github.com/mauri870/loopvec/internal/analysis" @@ -319,34 +319,25 @@ func addBuildTag(src []byte, tag string) []byte { return formatted } -// printDiff prints a unified diff between orig and rewritten for path, -// using the system diff command. Both sides are labeled with path, so the -// header does not depend on the temporary files or on which diff is installed -// (GNU and BSD diff print their timestamps differently). +// printDiff prints a unified diff between orig and rewritten for path, with +// both sides labeled with path. It needs no diff command, so the output is the +// same everywhere. func printDiff(path string, orig, rewritten []byte) error { - origFile, err := os.CreateTemp("", "loopvec-orig-*.go") - if err != nil { - return err - } - defer func() { _ = os.Remove(origFile.Name()) }() - if _, err := origFile.Write(orig); err != nil { - _ = origFile.Close() - return err - } - _ = origFile.Close() + return difflib.WriteUnifiedDiff(os.Stdout, difflib.UnifiedDiff{ + A: diffLines(orig), + B: diffLines(rewritten), + FromFile: path, + ToFile: path, + Context: 3, + }) +} - newFile, err := os.CreateTemp("", "loopvec-new-*.go") - if err != nil { - return err - } - defer func() { _ = os.Remove(newFile.Name()) }() - if _, err := newFile.Write(rewritten); err != nil { - _ = newFile.Close() - return err +// diffLines splits src into lines that keep their newline. difflib.SplitLines +// would add an empty line after a final newline. +func diffLines(src []byte) []string { + lines := strings.SplitAfter(string(src), "\n") + if lines[len(lines)-1] == "" { + lines = lines[:len(lines)-1] } - _ = newFile.Close() - - out, _ := exec.Command("diff", "-u", "-L", path, "-L", path, origFile.Name(), newFile.Name()).Output() - _, err = os.Stdout.Write(out) - return err + return lines } diff --git a/testdata/flag_diff.txt b/testdata/flag_diff.txt index 5d7bf4a..0bd5f39 100644 --- a/testdata/flag_diff.txt +++ b/testdata/flag_diff.txt @@ -1,16 +1,11 @@ -# -d prints a unified diff, labeled with the file's path on both sides so the -# headers are the same with GNU and BSD diff, and leaves the source untouched. +# -d prints a unified diff, labeled with the file's path on both sides, and +# leaves the source untouched. loopvec computes the diff itself, so the output +# does not depend on a diff command and is checked whole. cp ops.go ops.go.orig loopvec -d . cmpenv stderr stderr.want -stdout -count=1 '^--- \S+/ops\.go$' -stdout -count=1 '^\+\+\+ \S+/ops\.go$' -stdout -count=1 '^@@ -1,7 \+1,43 @@$' -stdout -count=1 '^\+//go:build goexperiment.simd$' -stdout -count=1 '^\+import \($' -stdout -count=1 '^- for i := range dst \{$' -stdout -count=1 '^\+ _v1.Add\(_v2\).StorePart\(dst\[_i:\]\)$' +cmpenv stdout stdout.want cmp ops.go ops.go.orig ! exists ops_simd.go @@ -28,3 +23,52 @@ func AddInt32s(dst, a, b []int32) { } -- stderr.want -- loopvec: rewrote 1 loop(s) in $WORK/ops.go +-- stdout.want -- +--- $WORK/ops.go ++++ $WORK/ops.go +@@ -1,7 +1,43 @@ ++//go:build goexperiment.simd ++ + package test + ++import ( ++ "simd" ++ "unsafe" ++) ++ + func AddInt32s(dst, a, b []int32) { +- for i := range dst { +- dst[i] = a[i] + b[i] ++ if _loopvecOverlap(dst, a) || _loopvecOverlap(dst, b) { ++ for i := range dst { ++ dst[i] = a[i] + b[i] ++ } ++ } else { ++ if len(dst) > 0 { ++ _ = a[len(dst)-1] ++ _ = b[len(dst)-1] ++ for _i := 0; _i < len(dst); { ++ _v1, _n := simd.LoadInt32sPart(a[_i:]) ++ _v2, _ := simd.LoadInt32sPart(b[_i:]) ++ _v1.Add(_v2).StorePart(dst[_i:]) ++ _i += _n ++ } ++ } + } + } ++ ++func _loopvecOverlap[T any](a, b []T, window ...int) bool { ++ aLo, aHi, bLo, bHi := 0, len(a), 0, len(b) ++ if len(window) == 4 { ++ aLo, aHi, bLo, bHi = window[0], window[1], window[2], window[3] ++ } ++ if aLo >= aHi || bLo >= bHi { ++ return false ++ } ++ size := unsafe.Sizeof(a[0]) ++ aBase := uintptr(unsafe.Pointer(unsafe.SliceData(a))) ++ bBase := uintptr(unsafe.Pointer(unsafe.SliceData(b))) ++ aStart, aEnd := aBase+uintptr(aLo)*size, aBase+uintptr(aHi)*size ++ bStart, bEnd := bBase+uintptr(bLo)*size, bBase+uintptr(bHi)*size ++ return aStart < bEnd && bStart < aEnd ++} From 895f862ae1a30f5efa542a6d559dacc5c8e1773d Mon Sep 17 00:00:00 2001 From: Mauri de Souza Meneguzzo Date: Tue, 3 Nov 2026 19:53:11 -0300 Subject: [PATCH 07/10] testdata: compare activation_functions.txt with the same machine's scalar build (#17) `activation_functions.txt` hashes float bits and compares both builds with `testmain/output`, which was recorded on amd64. It passes on arm64 only because none of its loops creates a NaN: they just pass the input NaN through. With an alpha of 0, `LeakyReLU` computes `0 * -Inf`. The resulting NaN has the sign bit set on amd64 and clear on arm64, so the script fails on arm64 against the recording. This change gives it #8's treatment: the recording is compared only on amd64, and on every target the simd build is compared with the scalar build of the same machine. It also changes the three scripts from #8: - They compared the simd build with the recording on amd64 as well. That adds nothing: on amd64 the scalar build matches the recording and the simd build matches the scalar build. - That comparison came last, so it was the one `-update` recorded. A simd build that differed from the scalar one would have been written into the recording. Now only the scalar build is compared with the recording, and that's what `-update` records. - Two of their comments named the square root of a negative number, though those scripts have no square root. Tested locally: - I made temporary copies of the old and new `activation_functions.txt` with alpha set to 0, recorded the expected output on amd64, and ran both on arm64 under qemu. The old script fails at `cmp stdout testmain/output`, and the new one passes. - The four scripts pass on amd64 and on arm64 under qemu, and `-update` leaves them unchanged. --- testdata/abs_sqrt.txt | 7 +++---- testdata/activation_functions.txt | 11 +++++++++-- testdata/activation_temp.txt | 9 ++++----- testdata/invariant_call.txt | 9 ++++----- 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/testdata/abs_sqrt.txt b/testdata/abs_sqrt.txt index d6368e2..5a44e70 100644 --- a/testdata/abs_sqrt.txt +++ b/testdata/abs_sqrt.txt @@ -5,9 +5,9 @@ # output is compared bit for bit. # testmain/output was recorded on amd64: a NaN that an operation creates (the square -# root of a negative number, 0 times infinity) has the sign bit set there and clear -# on arm64, so the recording is compared only on amd64, and on every target the -# simd build is compared with the scalar build of the same machine. +# root of a negative number) has the sign bit set there and clear on arm64, so only +# the scalar build is compared with the recording, and only on amd64. On every +# target the simd build is compared with the scalar build of the same machine. # The scalar program is the reference behavior. exec go run ./testmain @@ -24,7 +24,6 @@ cmp ops.go ops.go.guarded # The simd build must behave identically. env GOEXPERIMENT=simd exec go run ./testmain -[GOARCH:amd64] cmp stdout testmain/output cmp stdout scalar.out -- go.mod -- diff --git a/testdata/activation_functions.txt b/testdata/activation_functions.txt index d9cbac6..621bf43 100644 --- a/testdata/activation_functions.txt +++ b/testdata/activation_functions.txt @@ -3,9 +3,16 @@ # Both branches are computed for every lane, and the results must still match the # scalar loop for NaN, infinities and signed zeros, which compare the way Go does. +# testmain/output was recorded on amd64. These loops only pass the input NaN +# through, but a NaN that an operation creates (an alpha of 0 times an infinity) +# has the sign bit set there and clear on arm64, so only the scalar build is +# compared with the recording, and only on amd64. On every target the simd build +# is compared with the scalar build of the same machine. + # The scalar program is the reference behavior. exec go run ./testmain -cmp stdout testmain/output +[GOARCH:amd64] cmp stdout testmain/output +cp stdout scalar.out cp ops.go ops.go.orig loopvec -split . @@ -17,7 +24,7 @@ cmp ops.go ops.go.guarded # The simd build must behave identically. env GOEXPERIMENT=simd exec go run ./testmain -cmp stdout testmain/output +cmp stdout scalar.out -- go.mod -- module example.com/test diff --git a/testdata/activation_temp.txt b/testdata/activation_temp.txt index 5526f8d..6b65c05 100644 --- a/testdata/activation_temp.txt +++ b/testdata/activation_temp.txt @@ -4,10 +4,10 @@ # if/else if, then use it. Results must match the scalar loop bit for bit, NaN, # infinities and signed zeros included. -# testmain/output was recorded on amd64: a NaN that an operation creates (the square -# root of a negative number, 0 times infinity) has the sign bit set there and clear -# on arm64, so the recording is compared only on amd64, and on every target the -# simd build is compared with the scalar build of the same machine. +# testmain/output was recorded on amd64: a NaN that an operation creates (0 times +# infinity) has the sign bit set there and clear on arm64, so only the scalar build +# is compared with the recording, and only on amd64. On every target the simd build +# is compared with the scalar build of the same machine. exec go run ./testmain [GOARCH:amd64] cmp stdout testmain/output @@ -22,7 +22,6 @@ cmp ops.go ops.go.guarded env GOEXPERIMENT=simd exec go run ./testmain -[GOARCH:amd64] cmp stdout testmain/output cmp stdout scalar.out -- go.mod -- diff --git a/testdata/invariant_call.txt b/testdata/invariant_call.txt index 3f1fcf5..eafaef1 100644 --- a/testdata/invariant_call.txt +++ b/testdata/invariant_call.txt @@ -3,10 +3,10 @@ # the call is evaluated once before the loop. The results must match the scalar loop # bit for bit, which is the only way to compare NaN and negative zero. -# testmain/output was recorded on amd64: a NaN that an operation creates (the square -# root of a negative number, 0 times infinity) has the sign bit set there and clear -# on arm64, so the recording is compared only on amd64, and on every target the -# simd build is compared with the scalar build of the same machine. +# testmain/output was recorded on amd64: a NaN that an operation creates (0 times +# infinity) has the sign bit set there and clear on arm64, so only the scalar build +# is compared with the recording, and only on amd64. On every target the simd build +# is compared with the scalar build of the same machine. exec go run ./testmain [GOARCH:amd64] cmp stdout testmain/output @@ -21,7 +21,6 @@ cmp ops.go ops.go.guarded env GOEXPERIMENT=simd exec go run ./testmain -[GOARCH:amd64] cmp stdout testmain/output cmp stdout scalar.out -- go.mod -- From 917ead9004886bd16a0886807c3caf410c2cb8c0 Mon Sep 17 00:00:00 2001 From: Mauri de Souza Meneguzzo Date: Tue, 3 Nov 2026 21:38:12 -0300 Subject: [PATCH 08/10] BUGS.md: update the status of #80657 and the data for #80835 (#18) Issue 80657 is closed now that CL 839405 is in tip; 1.27.3 still crashes, which flag_methods_ice.txt pins. The section on 80835 said the effect on Intel was unknown. Others have since measured it in the issue: Skylake-X pays 2x to 3x on scalar code after an AVX-512 kernel, Sapphire Rapids and Granite Rapids lose about 15 points of peak on a kernel until ClearAVXUpperBits is added, and adding it to every short leaf function costs about 0.8 ns per call. Add those, along with loopvec's own measurements: GOAMD64=v3 does not remove the penalty, the clear costs nothing after a pure vector loop, and one rewritten fill made scalar loops in gonum's Dlantb 7x slower. loopvec does not emit the clear yet, so say "none yet". --- BUGS.md | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/BUGS.md b/BUGS.md index 55231c5..deb82fc 100644 --- a/BUGS.md +++ b/BUGS.md @@ -6,8 +6,8 @@ gotip, "1.27.3" is the release toolchain loopvec is tested against. | Issue | Status | Affects | loopvec's answer | |---|---|---|---| -| [#80657](https://github.com/golang/go/issues/80657) simd in a method: internal compiler error | fixed on tip (CL 839405), still crashes on 1.27.3 | `-methods` | methods are skipped unless `-methods` is passed | -| [#80835](https://github.com/golang/go/issues/80835) legacy SSE encodings in functions using simd cause AVX-SSE transition penalties | open, fix pending (CLs 825145, 825146, in tip) | speed of any scalar float code that runs after a rewritten loop, in the same function or another | none; the portable `simd` package has no `ClearAVXUpperBits` | +| [#80657](https://github.com/golang/go/issues/80657) simd in a method: internal compiler error | closed, fixed on tip (CL 839405), still crashes on 1.27.3 | `-methods` | methods are skipped unless `-methods` is passed | +| [#80835](https://github.com/golang/go/issues/80835) legacy SSE encodings in functions using simd cause AVX-SSE transition penalties | open, fix pending (CLs 825145, 825146, in tip) | speed of any scalar float code that runs after a rewritten loop, in the same function or another | none yet; the portable `simd` package has no `ClearAVXUpperBits` | | [#81844](https://github.com/golang/go/issues/81844) blank identifier parameters rejected | fixed on tip (CL 841145) | every rewrite of a function with a `_` parameter | the generated file renames them `_p0`, `_p1`, ... | | [#81845](https://github.com/golang/go/issues/81845) calls left unresolved when inlined into a package initializer | closed as a duplicate of [#80689](https://github.com/golang/go/issues/80689), which is open | loops in package-level `var x = func() {...}()` | not rewritten (the generated initializer fails to link), see `package_initializer_no_rewrite.txt` and `toolexec_package_initializer.txt` | | [#81846](https://github.com/golang/go/issues/81846) `Float32s.Neg` rebuilds its sign constant every iteration | open | speed of negation loops | none; the loop is correct, only slower than it should be | @@ -42,8 +42,21 @@ vector loop still wins and one that is mostly scalar float code around it can lo `simd/archsimd` (amd64) has `ClearAVXUpperBits`, not the portable `simd`, so loopvec has no portable way to clear the state. When comparing a function before and after a rewrite, benchmark the whole function, not the loop. This is separate from #81847: it reproduces with -async preemption off. One machine and no performance counters, so the size of the effect on -Intel, where the penalty works differently, is unknown. +async preemption off. + +Two more things from loopvec's runs, also on the 9950X3D. `GOAMD64=v3` does not remove the +penalty: the compiler still emits legacy SSE for scalar float code, except where it fuses a +multiply-add into a VEX `VFMADD231SD`. And `ClearAVXUpperBits()` costs nothing measurable +after a loop that stays in vectors (4096 `float32` adds, 255.3 against 254.0 ns). On real +code, one rewritten `work[i] = 1` fill in gonum's `Dlantb` made the scalar loops after it 7x +slower (420 to 2917 us), and the clear brought it back to 409 us. + +Others have measured Intel in the issue thread: Skylake-X pays 2x to 3x on scalar code after +an AVX-512 kernel, and on Sapphire Rapids and Granite Rapids a kernel that reloads with +legacy SSE loses about 15 points of peak until the clear is added. The same thread reports +`ClearAVXUpperBits()` at the return of every short leaf function costing about 0.8 ns per +call, so inserting it unconditionally is not free. loopvec does not emit it yet; it is +amd64-only and would need an arch-tagged helper. ## #81844: blank identifier parameters From 2c23922fc40eba0167c737ce9c1a5dd43127b76b Mon Sep 17 00:00:00 2001 From: thesyncim Date: Sat, 31 Oct 2026 21:39:13 +0000 Subject: [PATCH 09/10] internal/analysis: don't rewrite loops whose names the new code would capture The rewritten code adds names that begin with an underscore (_i, _count, _w1, broadcast vectors declared before the loop, ...) and relies on predeclared names (len, min, max, copy, uint64, the element types) and on the simd and unsafe packages. Each can mean something else in the function, and then the vector loop silently computes something else. All of these already go wrong on master: func F(dst, a []int32, _i int) { for i := 0; i < _i; i++ { ... } } reads loopvec's _i in the loop condition and never runs; with a package-level func len(s []int32) int the vector loop takes its window length from that len; and a reduction written as for n := range n, or for n := n - 1; n >= 0; n--, finishes with for n := _i; n < n; n++, which never runs. A loop that would otherwise be rewritten is now left alone, each with its own reason, when its function uses an identifier that begins with an underscore, other than _; when one of the names the code relies on, looked up from the loop, is declared again around it, in the package, or by an import; or when the loop declares such a name itself, or a name its range operand or initial values use. The names the overlap helper uses (uintptr, int, bool, any, false) are checked at package level, where loopvec declares it. The lookup needs scopes, so the toolexec wrapper records them. No loop in std, gonum or github.com/thesyncim/goav1 stops being vectorized. --- cmd/loopvec-toolexec/compile.go | 1 + internal/analysis/analyzer.go | 27 ++- internal/analysis/explain.go | 3 +- internal/loopir/lower.go | 116 ++++++++++++ internal/loopir/reason.go | 14 +- testdata/reserved_names.txt | 175 ++++++++++++++++++ ...lls_back_when_rewrite_does_not_compile.txt | 22 +-- 7 files changed, 339 insertions(+), 19 deletions(-) create mode 100644 testdata/reserved_names.txt diff --git a/cmd/loopvec-toolexec/compile.go b/cmd/loopvec-toolexec/compile.go index adf3ea4..82a6962 100644 --- a/cmd/loopvec-toolexec/compile.go +++ b/cmd/loopvec-toolexec/compile.go @@ -328,6 +328,7 @@ func check(fset *token.FileSet, files []*ast.File, c compileArgs, cfg *importcfg Defs: make(map[*ast.Ident]types.Object), Uses: make(map[*ast.Ident]types.Object), Selections: make(map[*ast.SelectorExpr]*types.Selection), + Scopes: make(map[ast.Node]*types.Scope), } var errs []error goarch := os.Getenv("GOARCH") diff --git a/internal/analysis/analyzer.go b/internal/analysis/analyzer.go index d686b59..cacc065 100644 --- a/internal/analysis/analyzer.go +++ b/internal/analysis/analyzer.go @@ -42,9 +42,10 @@ func Analyze(file *ast.File, info *types.Info, opts Options) []*loopir.Plan { continue } results := loopir.NamedResults(fn, info) + reserved := loopir.UsesReservedNames(fn) ast.Inspect(fn.Body, func(n ast.Node) bool { if stmt, ok := n.(ast.Stmt); ok { - if plan, _ := analyzeLoop(stmt, info, results, opts.Plan()); plan != nil { + if plan, _ := analyzeLoop(stmt, info, info.Scopes[file], results, reserved, opts.Plan()); plan != nil { plans = append(plans, plan) } } @@ -55,8 +56,10 @@ func Analyze(file *ast.File, info *types.Info, opts Options) []*loopir.Plan { } // analyzeLoop lowers stmt, which must be a for or range statement, and plans -// it. It returns the reason when the loop is not vectorizable. -func analyzeLoop(stmt ast.Stmt, info *types.Info, results map[types.Object]bool, plan loopir.Options) (*loopir.Plan, loopir.Reason) { +// it. It returns the reason when the loop is not vectorizable. reserved is set +// when the enclosing function uses a name that begins with an underscore (see +// loopir.UsesReservedNames); file is the scope of the file stmt is in. +func analyzeLoop(stmt ast.Stmt, info *types.Info, file *types.Scope, results map[types.Object]bool, reserved bool, plan loopir.Options) (*loopir.Plan, loopir.Reason) { switch stmt.(type) { case *ast.RangeStmt, *ast.ForStmt: default: @@ -66,5 +69,21 @@ func analyzeLoop(stmt ast.Stmt, info *types.Info, results map[types.Object]bool, if reason != "" { return nil, reason } - return loopir.NewPlan(loop, plan) + p, reason := loopir.NewPlan(loop, plan) + if p == nil { + return nil, reason + } + // The names are checked last, so that a loop that cannot be vectorized + // anyway keeps the reason that matters. + if reserved { + return nil, loopir.ReasonReservedName + } + var scope *types.Scope + if file != nil { + scope = file.Innermost(stmt.Pos()) + } + if loopir.ShadowsEmittedName(scope, stmt.Pos()) || loopir.HeaderShadowsName(stmt) { + return nil, loopir.ReasonShadowedName + } + return p, reason } diff --git a/internal/analysis/explain.go b/internal/analysis/explain.go index 10a47ec..c0f4017 100644 --- a/internal/analysis/explain.go +++ b/internal/analysis/explain.go @@ -32,6 +32,7 @@ func AnalyzeExplain(fset *token.FileSet, file *ast.File, info *types.Info, opts } isMethod := fn.Recv != nil results := loopir.NamedResults(fn, info) + reserved := loopir.UsesReservedNames(fn) ast.Inspect(fn.Body, func(n ast.Node) bool { var body *ast.BlockStmt switch stmt := n.(type) { @@ -54,7 +55,7 @@ func AnalyzeExplain(fset *token.FileSet, file *ast.File, info *types.Info, opts return true } - plan, reason := analyzeLoop(n.(ast.Stmt), info, results, opts.Plan()) + plan, reason := analyzeLoop(n.(ast.Stmt), info, info.Scopes[file], results, reserved, opts.Plan()) c.Vectorized = plan != nil c.Reason = reason out = append(out, c) diff --git a/internal/loopir/lower.go b/internal/loopir/lower.go index dc84301..cf9e6d9 100644 --- a/internal/loopir/lower.go +++ b/internal/loopir/lower.go @@ -7,6 +7,7 @@ import ( "go/token" "go/types" "math" + "slices" "strconv" "strings" ) @@ -405,6 +406,121 @@ func isIntType(t types.Type) bool { return ok && (basic.Kind() == types.Int || basic.Kind() == types.UntypedInt) } +// UsesReservedNames reports whether fn mentions an identifier that begins with +// an underscore, other than the blank identifier: every name loopvec adds does, +// so code it inserts into fn could capture one. +func UsesReservedNames(fn *ast.FuncDecl) bool { + found := false + ast.Inspect(fn, func(n ast.Node) bool { + if id, ok := n.(*ast.Ident); ok && len(id.Name) > 1 && id.Name[0] == '_' { + found = true + } + return !found + }) + return found +} + +// emittedNames are the predeclared and package names the rewritten code uses: +// len, min, max and copy, uint64 for shift counts, the element types of +// reductions (byte and rune among them, as go/types spells those), and the +// simd and unsafe packages. +var emittedNames = []string{ + "len", "min", "max", "copy", "uint64", + "int8", "int16", "int32", "int64", "uint8", "uint16", "uint32", "float32", "float64", + "byte", "rune", + "simd", "unsafe", +} + +// HeaderShadowsName reports whether a variable stmt, a for or range statement, +// declares in its header could capture a name of the rewritten code. The +// scalar loop that finishes a reduction declares the same variables as stmt and +// evaluates the bound inside their scope, while stmt evaluates its range +// operand, or the start it counts down from, outside: an index named len would +// break len(a), and for n := range n or for n := n - 1; n >= 0; n-- would +// compare the index with itself. So stmt must not declare one of emittedNames, +// nor a name used by its range operand or its initial values. +func HeaderShadowsName(stmt ast.Stmt) bool { + var vars, outside []ast.Expr + switch s := stmt.(type) { + case *ast.RangeStmt: + if s.Tok == token.DEFINE { + vars = []ast.Expr{s.Key, s.Value} + } + outside = []ast.Expr{s.X} + case *ast.ForStmt: + if init, ok := s.Init.(*ast.AssignStmt); ok && init.Tok == token.DEFINE { + vars, outside = init.Lhs, init.Rhs + } + } + for _, v := range vars { + id, ok := v.(*ast.Ident) + if !ok || id.Name == "_" { + continue + } + if slices.Contains(emittedNames, id.Name) { + return true + } + for _, e := range outside { + if mentions(e, id.Name) { + return true + } + } + } + return false +} + +// mentions reports whether e uses an identifier named name. +func mentions(e ast.Expr, name string) bool { + found := false + ast.Inspect(e, func(n ast.Node) bool { + if id, ok := n.(*ast.Ident); ok && id.Name == name { + found = true + } + return !found + }) + return found +} + +// helperNames are the predeclared names that _loopvecOverlap, which loopvec +// declares at package level, uses besides emittedNames. +var helperNames = []string{"any", "bool", "false", "int", "uintptr"} + +// ShadowsEmittedName reports whether, at pos in scope, a name the rewritten code +// uses means something else than the code needs: a predeclared name declared +// again in an enclosing scope, the package or an import, or simd or unsafe naming +// something other than those packages. scope is the innermost scope containing +// pos, or nil when the type information has no scopes, which counts as shadowed. +// The names only the overlap helper uses matter at package level, where it is +// declared. +func ShadowsEmittedName(scope *types.Scope, pos token.Pos) bool { + if scope == nil { + return true + } + pkg := scope + for pkg.Parent() != nil && pkg.Parent() != types.Universe { + pkg = pkg.Parent() + } + for _, name := range helperNames { + if pkg.Lookup(name) != nil { + return true + } + } + for _, name := range emittedNames { + _, obj := scope.LookupParent(name, pos) + switch name { + case "simd", "unsafe": + if pkg, ok := obj.(*types.PkgName); obj != nil && (!ok || pkg.Imported().Path() != name) { + return true + } + default: + if obj != types.Universe.Lookup(name) { + return true + } + } + } + return false +} + // NamedResults returns the named result variables of fn and of every function // literal inside it. func NamedResults(fn *ast.FuncDecl, info *types.Info) map[types.Object]bool { diff --git a/internal/loopir/reason.go b/internal/loopir/reason.go index 50e049c..20dfb66 100644 --- a/internal/loopir/reason.go +++ b/internal/loopir/reason.go @@ -107,6 +107,18 @@ const ( // propagate the NaN and order the zeros. arm64 matches Go, so the result // would depend on the machine. ReasonFloatMinMax Reason = "float min and max differ from Go's on some architectures (NaN and signed zero)" + // ReasonReservedName covers a loop in a function that uses an identifier + // beginning with an underscore, other than _ itself. Every name loopvec adds + // begins with one (_i, _count, _w1, _v1, _vcFloat32s0, _sx_f, ...), and code + // it adds before or inside the loop could shadow the function's own and change + // what it refers to. + ReasonReservedName Reason = "the function uses a name that begins with an underscore, as loopvec's own names do" + // ReasonShadowedName covers a loop where a predeclared name the rewritten + // code uses (len, min, max, copy, uint64, an element type) is declared again + // in a scope around it, its package or an import, or where simd or unsafe + // names something other than those packages: the code would call or convert + // with the wrong thing. + ReasonShadowedName Reason = "a name the rewritten code relies on (len, min, simd, ...) means something else here" // ReasonMethodSkipped covers a loop inside a method, skipped unless // -methods is passed (see analysis.Analyze's doc comment). ReasonMethodSkipped Reason = "loops inside methods are skipped without -methods" @@ -124,7 +136,7 @@ const ( // and "lower" for everything about the loop's own shape. func (r Reason) Stage() string { switch r { - case ReasonMethodSkipped: + case ReasonMethodSkipped, ReasonReservedName, ReasonShadowedName: return "analysis" case ReasonUnsupportedOp, ReasonFloatMinMax, ReasonFloatReduction, ReasonMixedTypes, ReasonTooManySlices, ReasonShortLoop: return "plan" diff --git a/testdata/reserved_names.txt b/testdata/reserved_names.txt new file mode 100644 index 0000000..2a98eaa --- /dev/null +++ b/testdata/reserved_names.txt @@ -0,0 +1,175 @@ +# Every name loopvec adds begins with an underscore, and some are declared before +# the loop or inside it, where they would shadow the function's own: a parameter +# named _w1 or _count, a limit named _i, or a local named like a broadcast vector +# would then refer to loopvec's variable, so a function that uses such a name is +# left alone; the blank identifier is not such a name. The code also relies on +# predeclared names (len, min, copy, uint64, ...) and on the packages simd and +# unsafe: a loop where one of them, looked up from the loop, is a local, a +# package-level declaration or an import of something else is left alone too, +# while a declaration the loop cannot see (max after it) does not matter. The +# scalar loop that finishes a reduction redeclares the loop's variables around +# its bound, so a loop variable must not take one of those names either, nor a +# name used by the range operand or the start of a loop that counts down +# (for n := range n, for n := n - 1; n >= 0; n--). The overlap helper loopvec +# declares in the package relies on a few more predeclared names (uintptr, +# ...), which the package must not redeclare. The rewritten package must build. + +loopvec -json ./... +cmpenv stdout stdout.want + +loopvec -split . +cmpenv stderr stderr.want +env GOEXPERIMENT=simd +exec go build . + +-- go.mod -- +module example.com/test + +go 1.27.1 +-- ops.go -- +package test + +func WindowName(dst, _w1 []int32) { + for i := range dst { + dst[i] = _w1[i] + 1 + } +} + +func CountName(dst, a []int32, _count int) { + for i := 0; i < len(dst); i++ { + dst[i] = a[i+_count] + 1 + } +} + +func IndexName(dst, a []int32, _i int) { + for i := 0; i < _i; i++ { + dst[i] = a[i] + 1 + } +} + +func BroadcastName(dst []float32, c float32) float32 { + _vcFloat32s0 := c * 2 + for i := range dst { + dst[i] *= c + } + return _vcFloat32s0 +} + +func Blank(dst, a []int32, _ int) { + for i := range dst { + dst[i] = a[i] + 1 + } +} + +func LocalLen(dst, a []int32) { + len := func(s []int32) int { return 2 } + _ = len + for i := range dst { + dst[i] = a[i] + 1 + } +} + +func LocalMin(dst, a []int32, k int) { + min := func(x, y int) int { return 0 } + _ = min + for i := 0; i < 8; i++ { + dst[i] = a[i+k] + 1 + } +} + +func MaxAfterLoop(dst, a []int32) int32 { + for i := range dst { + dst[i] = a[i] + 1 + } + max := int32(0) + for _, v := range dst { + if v > max { + max = v + } + } + return max +} +func SumCount(a []int32, n int) int32 { + var s int32 + for n := range n { + s += a[n] + } + return s +} + +func SumIndexLen(a []int32) int32 { + var s int32 + for len := range a { + s += a[len] + } + return s +} + +func SumDown(a []int32, n int) int32 { + var s int32 + for n := n - 1; n >= 0; n-- { + s += a[n] + } + return s +} + +func SumByte(a []byte) byte { + var s byte + { + type byte = int8 + var _ byte + for _, x := range a { + s += x + } + } + return s +} +-- pkglen/ops.go -- +package pkglen + +func len(s []int32) int { return cap(s) / 4 } + +func Double(dst, a []int32, n int) { + for i := 0; i < n; i++ { + dst[i] = a[i] * 2 + } +} +-- pkguintptr/ops.go -- +package pkguintptr + +type uintptr = int32 + +func Inc(dst, a []int32) { + for i := range dst { + dst[i] = a[i] + 1 + } +} +-- alias/ops.go -- +package alias + +import simd "strings" + +var _ = simd.ToUpper + +func Double(dst, a []int32) { + for i := range dst { + dst[i] = a[i] * 2 + } +} +-- stdout.want -- +{"file":"$WORK/ops.go","line":4,"func":"WindowName","vectorized":false,"reason":"the function uses a name that begins with an underscore, as loopvec's own names do","stage":"analysis"} +{"file":"$WORK/ops.go","line":10,"func":"CountName","vectorized":false,"reason":"the function uses a name that begins with an underscore, as loopvec's own names do","stage":"analysis"} +{"file":"$WORK/ops.go","line":16,"func":"IndexName","vectorized":false,"reason":"the function uses a name that begins with an underscore, as loopvec's own names do","stage":"analysis"} +{"file":"$WORK/ops.go","line":23,"func":"BroadcastName","vectorized":false,"reason":"the function uses a name that begins with an underscore, as loopvec's own names do","stage":"analysis"} +{"file":"$WORK/ops.go","line":30,"func":"Blank","vectorized":true} +{"file":"$WORK/ops.go","line":38,"func":"LocalLen","vectorized":false,"reason":"a name the rewritten code relies on (len, min, simd, ...) means something else here","stage":"analysis"} +{"file":"$WORK/ops.go","line":46,"func":"LocalMin","vectorized":false,"reason":"a name the rewritten code relies on (len, min, simd, ...) means something else here","stage":"analysis"} +{"file":"$WORK/ops.go","line":52,"func":"MaxAfterLoop","vectorized":true} +{"file":"$WORK/ops.go","line":65,"func":"SumCount","vectorized":false,"reason":"a name the rewritten code relies on (len, min, simd, ...) means something else here","stage":"analysis"} +{"file":"$WORK/ops.go","line":73,"func":"SumIndexLen","vectorized":false,"reason":"a name the rewritten code relies on (len, min, simd, ...) means something else here","stage":"analysis"} +{"file":"$WORK/ops.go","line":81,"func":"SumDown","vectorized":false,"reason":"a name the rewritten code relies on (len, min, simd, ...) means something else here","stage":"analysis"} +{"file":"$WORK/alias/ops.go","line":8,"func":"Double","vectorized":false,"reason":"a name the rewritten code relies on (len, min, simd, ...) means something else here","stage":"analysis"} +{"file":"$WORK/pkglen/ops.go","line":6,"func":"Double","vectorized":false,"reason":"a name the rewritten code relies on (len, min, simd, ...) means something else here","stage":"analysis"} +{"file":"$WORK/pkguintptr/ops.go","line":6,"func":"Inc","vectorized":false,"reason":"a name the rewritten code relies on (len, min, simd, ...) means something else here","stage":"analysis"} +-- stderr.want -- +loopvec: rewrote 2 loop(s) in $WORK/ops.go diff --git a/testdata/toolexec_falls_back_when_rewrite_does_not_compile.txt b/testdata/toolexec_falls_back_when_rewrite_does_not_compile.txt index e8555d5..f87bc51 100644 --- a/testdata/toolexec_falls_back_when_rewrite_does_not_compile.txt +++ b/testdata/toolexec_falls_back_when_rewrite_does_not_compile.txt @@ -1,6 +1,6 @@ # A rewrite that does not type-check is discarded: the package is compiled from # its original source, the build succeeds, and the log says why. Here the -# parameter named simd shadows the package the rewrite imports. +# package already declares _loopvecOverlap, the helper the rewrite adds. env GOEXPERIMENT=simd env GOCACHE=$WORK/gocache @@ -9,7 +9,7 @@ env LOOPVEC_TOOLEXEC_LOG_PKGS=example.com/app/vec exec go build -toolexec=$LOOPVEC_TOOLEXEC -o app . ! stdout . ! stderr . -cmpenv log log.want +grep -count=1 '^kept example\.com/app/vec: rewritten source rejected: .*/vec/vec\.go:[0-9]+:[0-9]+: _loopvecOverlap redeclared in this block$' log exec ./app cmp stdout want.out @@ -18,17 +18,15 @@ cmp stdout want.out module example.com/app go 1.27.1 --- vec/vec.go -- +-- vec/helper.go -- package vec -// The parameter named simd shadows the package the rewrite would import. -func AddInt32s(simd int, dst, a, b []int32) { - for i := range dst { - dst[i] = a[i] + b[i] + int32(simd) - } -} +// _loopvecOverlap has the name of the helper the rewrite would add. +func _loopvecOverlap() {} +-- vec/vec.go -- +package vec -func Add(simd int, dst, a, b []int32) { +func Add(dst, a, b []int32) { for i := range dst { dst[i] = a[i] + b[i] } @@ -44,10 +42,8 @@ import ( func main() { d := make([]int32, 3) - vec.Add(0, d, []int32{1, 2, 3}, []int32{10, 20, 30}) + vec.Add(d, []int32{1, 2, 3}, []int32{10, 20, 30}) fmt.Println(d) } -- want.out -- [11 22 33] --- log.want -- -kept example.com/app/vec: rewritten source rejected: $WORK/vec/vec.go:10:22: simd.BroadcastInt32s undefined (type int has no field or method BroadcastInt32s) From 2eab9ea7fbfa9b94fa07ba637e5b80c2d7931276 Mon Sep 17 00:00:00 2001 From: thesyncim Date: Sat, 31 Oct 2026 23:06:32 +0000 Subject: [PATCH 10/10] internal/rewrite: rename blank parameters only in rewritten functions fixBlankParams works around a compiler bug that rejects _ parameters in functions with simd code by renaming them _p0, _p1, ..., but it renamed them in every function of a rewritten file. In a function that reads a package-level _p0, the parameter then hid it: var _p0 = 5 func Offset(_ int) int { return _p0 } became func Offset(_p0 int) int { return _p0 } and returned its argument. Only the functions loopvec rewrote are renamed now. They mention no name that begins with an underscore, so the new names cannot capture anything. --- internal/rewrite/rewriter.go | 44 ++++++++++++++++++-------- testdata/blank_params.txt | 60 ++++++++++++++++++++++++++++++++++-- 2 files changed, 89 insertions(+), 15 deletions(-) diff --git a/internal/rewrite/rewriter.go b/internal/rewrite/rewriter.go index 558e835..25fb69c 100644 --- a/internal/rewrite/rewriter.go +++ b/internal/rewrite/rewriter.go @@ -74,6 +74,10 @@ func File(fset *token.FileSet, file *ast.File, info *types.Info, src []byte, opt } needsOverlapHelper := false needsSimd := false + // rewritten holds the functions given a rewritten loop, by their index + // among the file's function declarations. Rewriting changes only loop + // bodies and appends the overlap helper, so the indexes stay valid. + rewritten := map[int]bool{} for _, plan := range plans { stmtNode := plan.Loop.Node @@ -106,6 +110,11 @@ func File(fset *token.FileSet, file *ast.File, info *types.Info, src []byte, opt end: stmtNode.End(), text: repl, }) + for i, fd := range funcDecls(file) { + if fd.Pos() <= stmtNode.Pos() && stmtNode.End() <= fd.End() { + rewritten[i] = true + } + } if pre != "" { extraStmts = append(extraStmts, struct { pos token.Pos @@ -179,7 +188,7 @@ func File(fset *token.FileSet, file *ast.File, info *types.Info, src []byte, opt // Rename blank identifier parameters to avoid a gotip GOEXPERIMENT=simd // compiler bug where _ params in simd-using functions produce // "cannot use _ as value or type". - formatted = fixBlankParams(fset, formatted) + formatted = fixBlankParams(fset, formatted, rewritten) return Result{Src: formatted, Rewrites: len(replacements), DefinesHelper: definesHelper}, nil } @@ -307,9 +316,12 @@ func ensureOverlapHelper(src []byte) []byte { // fixBlankParams renames blank identifier (_) function parameters in src to // _p0, _p1, … to work around a gotip GOEXPERIMENT=simd compiler bug: // blank params in functions that contain simd code cause the SIMD lowering -// pass to emit "cannot use _ as value or type". The rename is safe because -// the parameters are never referenced by name in the body. -func fixBlankParams(fset *token.FileSet, src []byte) []byte { +// pass to emit "cannot use _ as value or type". Only the functions loopvec +// rewrote, given by their funcDecls index in rewritten, are renamed: they +// mention no name that begins with an underscore, so the new names cannot +// capture anything, while in another function a _p0 parameter could hide a +// package-level _p0. +func fixBlankParams(fset *token.FileSet, src []byte, rewritten map[int]bool) []byte { file, err := parser.ParseFile(fset, "", src, parser.ParseComments) if err != nil { return src @@ -317,13 +329,9 @@ func fixBlankParams(fset *token.FileSet, src []byte) []byte { counter := 0 changed := false - ast.Inspect(file, func(n ast.Node) bool { - fd, ok := n.(*ast.FuncDecl) - if !ok { - return true - } - if fd.Type.Params == nil { - return true + for i, fd := range funcDecls(file) { + if fd.Type.Params == nil || !rewritten[i] { + continue } for _, field := range fd.Type.Params.List { for _, name := range field.Names { @@ -334,8 +342,7 @@ func fixBlankParams(fset *token.FileSet, src []byte) []byte { } } } - return true - }) + } if !changed { return src @@ -348,6 +355,17 @@ func fixBlankParams(fset *token.FileSet, src []byte) []byte { return buf.Bytes() } +// funcDecls returns the function declarations of file in source order. +func funcDecls(file *ast.File) []*ast.FuncDecl { + var fds []*ast.FuncDecl + for _, decl := range file.Decls { + if fd, ok := decl.(*ast.FuncDecl); ok { + fds = append(fds, fd) + } + } + return fds +} + // ensureImport adds an import of path to the source if not already present. func ensureImport(src []byte, path string) ([]byte, error) { fset := token.NewFileSet() diff --git a/testdata/blank_params.txt b/testdata/blank_params.txt index 1413dbf..a2c424c 100644 --- a/testdata/blank_params.txt +++ b/testdata/blank_params.txt @@ -1,6 +1,9 @@ # Blank identifier parameters are renamed to _p0, _p1, ... to avoid a gotip # GOEXPERIMENT=simd compiler bug where _ params in simd-using functions produce -# "cannot use _ as value or type". +# "cannot use _ as value or type". Only the functions loopvec rewrites are +# renamed: Offset reads the package-level _p0, which a parameter of that name +# would hide, and so do Local, which also has a variable named simd, and the +# first blank function. cp ops.go ops.go.orig loopvec -split . @@ -16,11 +19,28 @@ go 1.27.1 -- ops.go -- package test +var _p0 = 5 + +func Offset(_ int) int { return _p0 } + +func Local(_ int) int { + simd := struct{ x int }{1} + return _p0 + simd.x +} + +func _(_ int) int { return _p0 } + func FillBlankParams(_ int, _ byte, dst []float32) { for i := range dst { dst[i] = 1 } } + +func _(_ int, dst []float32) { + for i := range dst { + dst[i] = 2 + } +} -- ops_simd.go.want -- //go:build goexperiment.simd @@ -28,6 +48,17 @@ package test import "simd" +var _p0 = 5 + +func Offset(_ int) int { return _p0 } + +func Local(_ int) int { + simd := struct{ x int }{1} + return _p0 + simd.x +} + +func _(_ int) int { return _p0 } + func FillBlankParams(_p0 int, _p1 byte, dst []float32) { _vcFloat32s0 := simd.BroadcastFloat32s(1) for _i := 0; _i < len(dst); { @@ -35,15 +66,40 @@ func FillBlankParams(_p0 int, _p1 byte, dst []float32) { _i += _n } } + +func _(_p2 int, dst []float32) { + _vcFloat32s1 := simd.BroadcastFloat32s(2) + for _i := 0; _i < len(dst); { + _n := _vcFloat32s1.StorePart(dst[_i:]) + _i += _n + } +} -- stderr.want -- -loopvec: rewrote 1 loop(s) in $WORK/ops.go +loopvec: rewrote 2 loop(s) in $WORK/ops.go -- ops.go.guarded -- //go:build !goexperiment.simd package test +var _p0 = 5 + +func Offset(_ int) int { return _p0 } + +func Local(_ int) int { + simd := struct{ x int }{1} + return _p0 + simd.x +} + +func _(_ int) int { return _p0 } + func FillBlankParams(_ int, _ byte, dst []float32) { for i := range dst { dst[i] = 1 } } + +func _(_ int, dst []float32) { + for i := range dst { + dst[i] = 2 + } +}