From 00263c00577155ad5207ce1efde8d254fa2aadec Mon Sep 17 00:00:00 2001 From: thesyncim Date: Sat, 31 Oct 2026 18:21:49 +0000 Subject: [PATCH 1/3] internal/loopir: reject a constant offset that puts an index below zero A read at a negative constant offset in a loop with a constant limit, such as dst[i] = a[i-20] + 1 in a loop that stops before 16, was rewritten into code that does not compile: the length check of the vector loop is _ = a[16-21], a negative constant index. -split wrote that file, and the toolexec wrapper fell back to the original package. Such a loop panics as soon as it runs, as does one whose first index is a negative constant (a[i-1] in a loop from 0). Both are now left alone with their own reason, the way a constant limit of zero or less already is. A read whose indexes can all be zero or more (a[i-1] from 1) is still rewritten. The checks read the start and limit as text, so a named constant or a constant expression (const s0 = 0; for i := s0; ...) slipped past them. A constant start or limit is now spelled as its value, which also lets the short-loop rule see a named constant trip count: goav1's for x := range restorationExtraHorz, three iterations, is no longer rewritten. The vectorized set of std and gonum does not change. --- 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 f000a1af429b063181b4497084d5eaa6174a55cf Mon Sep 17 00:00:00 2001 From: thesyncim Date: Sat, 31 Oct 2026 21:39:13 +0000 Subject: [PATCH 2/3] 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 13f0d91..999160f 100644 --- a/cmd/loopvec-toolexec/compile.go +++ b/cmd/loopvec-toolexec/compile.go @@ -321,6 +321,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 8d6eb6e..f9df98a 100644 --- a/internal/loopir/lower.go +++ b/internal/loopir/lower.go @@ -7,6 +7,7 @@ import ( "go/token" "go/types" "math" + "slices" "strconv" "strings" ) @@ -376,6 +377,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 4757287..a9616d9 100644 --- a/internal/loopir/reason.go +++ b/internal/loopir/reason.go @@ -102,6 +102,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" @@ -119,7 +131,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 9087cb677b50d525a161fdc9ba325856d5e36ce4 Mon Sep 17 00:00:00 2001 From: thesyncim Date: Sat, 31 Oct 2026 23:06:32 +0000 Subject: [PATCH 3/3] 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 + } +}