Skip to content

internal/analysis: don't rewrite loops whose names the new code would capture - #3

Open
thesyncim wants to merge 3 commits into
mauri870:masterfrom
thesyncim:reserved-names
Open

thesyncim wants to merge 3 commits into
mauri870:masterfrom
thesyncim:reserved-names

Conversation

@thesyncim

@thesyncim thesyncim commented Nov 1, 2026 •

Copy link
Copy Markdown

Stacked on #2; this PR's own changes.

The rewritten code adds names that start with an underscore, some of them declared before or inside the loop. It also calls len, min, max and copy, converts with uint64 and the element types, and refers to the packages simd and unsafe. The function, its package or an import can give any of those names another meaning, and then the vector loop silently computes something else. These all go wrong on master:

func F(dst, a []int32, _i int) {
	for i := 0; i < _i; i++ {
		dst[i] = a[i] + 1
	}
}

The vector loop's condition reads loopvec's _i instead of the parameter, so it never runs. In a package that declares func len(s []int32) int { return cap(s) / 4 }, the vector loop takes its length from that len and stops a quarter of the way. And a sum written as for n := range n { s += a[n] }, or as for n := n - 1; n >= 0; n--, finishes its last elements with for n := _i; n < n; n++, which never runs, so the result is short.

A loop that would otherwise be rewritten is now left alone, with its own reason, when:

  • its function uses an identifier that starts with an underscore, other than _. Rather than track which names each part of the emitter uses, this covers the names loopvec adds now and any it adds later;
  • one of the names the code relies on, looked up from the loop, resolves to something other than the predeclared one or the imported package. The lookup starts at the loop's position, so a max declared after the loop doesn't count. The overlap helper's own names (uintptr, int, bool, any, false) are checked at package level, where it is declared. The toolexec wrapper now records scopes for this;
  • the loop itself declares one of those names, or a name its range operand or initial values use (for n := range n, for n := n - 1; ..., for len := range a).

These checks run last, so a loop that can't be vectorized anyway keeps the reason that matters.

The second commit fixes the same kind of bug in fixBlankParams. It renames _ parameters to _p0, _p1, ... to work around a compiler bug with simd code, but it did that in every function of a rewritten file, so func Offset(_ int) int { return _p0 } started returning its argument instead of a package-level _p0. Now it only renames in the functions loopvec rewrote, which can't mention such names.

No loop in std, gonum or goav1 stops being vectorized. reserved_names.txt and blank_params.txt cover each case. A rewrite that doesn't type-check still falls back in toolexec; toolexec_falls_back_when_rewrite_does_not_compile.txt used a parameter named simd to get one, which is now rejected earlier, so it uses a package that declares _loopvecOverlap itself instead.

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.
… 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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant