From 837ed98d631928ff0036df03899b83c50237555f Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Thu, 22 Mar 2018 12:04:51 -0400 Subject: [PATCH] cmd/compile: don't produce a past-the-end pointer in range loops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently, range loops over slices and arrays are compiled roughly like: for i, x := range s { b } ⇓ for i, _n, _p := 0, len(s), &s[0]; i < _n; i, _p = i+1, _p + unsafe.Sizeof(s[0]) { b } ⇓ i, _n, _p := 0, len(s), &s[0] goto cond body: { b } i, _p = i+1, _p + unsafe.Sizeof(s[0]) cond: if i < _n { goto body } else { goto end } end: The problem with this lowering is that _p may temporarily point past the end of the allocation the moment before the loop terminates. Right now this isn't a problem because there's never a safe-point during this brief moment. We're about to introduce safe-points everywhere, so this bad pointer is going to be a problem. We could mark the increment as an unsafe block, but this inhibits reordering opportunities and could result in infrequent safe-points if the body is short. Instead, this CL fixes this by changing how we compile range loops to never produce this past-the-end pointer. It changes the lowering to roughly: i, _n, _p := 0, len(s), &s[0] if i < _n { goto body } else { goto end } top: _p += unsafe.Sizeof(s[0]) body: { b } i++ if i < _n { goto top } else { goto end } end: Notably, the increment is split into two parts: we increment the index before checking the condition, but increment the pointer only *after* the condition check has succeeded. The implementation builds on the OFORUNTIL construct that was introduced during the loop preemption experiments, since OFORUNTIL places the increment and condition after the loop body. To support the extra "late increment" step, we further define OFORUNTIL's "List" field to contain the late increment statements. This makes all of this a relatively small change. This depends on the improvements to the prove pass in CL 102603. With the current lowering, bounds-check elimination knows that i < _n in the body because the body block is dominated by the cond block. In the new lowering, deriving this fact requires detecting that i < _n on *both* paths into body and hence is true in body. CL 102603 made prove able to detect this. The code size effect of this is minimal. The cmd/go binary on linux/amd64 increases by 0.17%. Performance-wise, this actually appears to be a net win, though it's mostly noise: name old time/op new time/op delta BinaryTree17-12 2.80s ± 0% 2.61s ± 1% -6.88% (p=0.000 n=20+18) Fannkuch11-12 2.41s ± 0% 2.42s ± 0% +0.05% (p=0.005 n=20+20) FmtFprintfEmpty-12 41.6ns ± 5% 41.4ns ± 6% ~ (p=0.765 n=20+19) FmtFprintfString-12 69.4ns ± 3% 69.3ns ± 1% ~ (p=0.084 n=19+17) FmtFprintfInt-12 76.1ns ± 1% 77.3ns ± 1% +1.57% (p=0.000 n=19+19) FmtFprintfIntInt-12 122ns ± 2% 123ns ± 3% +0.95% (p=0.015 n=20+20) FmtFprintfPrefixedInt-12 153ns ± 2% 151ns ± 3% -1.27% (p=0.013 n=20+20) FmtFprintfFloat-12 215ns ± 0% 216ns ± 0% +0.47% (p=0.000 n=20+16) FmtManyArgs-12 486ns ± 1% 498ns ± 0% +2.40% (p=0.000 n=20+17) GobDecode-12 6.43ms ± 0% 6.50ms ± 0% +1.08% (p=0.000 n=18+19) GobEncode-12 5.43ms ± 1% 5.47ms ± 0% +0.76% (p=0.000 n=20+20) Gzip-12 218ms ± 1% 218ms ± 1% ~ (p=0.883 n=20+20) Gunzip-12 38.8ms ± 0% 38.9ms ± 0% ~ (p=0.644 n=19+19) HTTPClientServer-12 76.2µs ± 1% 76.4µs ± 2% ~ (p=0.218 n=20+20) JSONEncode-12 12.2ms ± 0% 12.3ms ± 1% +0.45% (p=0.000 n=19+19) JSONDecode-12 54.2ms ± 1% 53.3ms ± 0% -1.67% (p=0.000 n=20+20) Mandelbrot200-12 3.71ms ± 0% 3.71ms ± 0% ~ (p=0.143 n=19+20) GoParse-12 3.22ms ± 0% 3.19ms ± 1% -0.72% (p=0.000 n=20+20) RegexpMatchEasy0_32-12 76.7ns ± 1% 75.8ns ± 1% -1.19% (p=0.000 n=20+17) RegexpMatchEasy0_1K-12 245ns ± 1% 243ns ± 0% -0.72% (p=0.000 n=18+17) RegexpMatchEasy1_32-12 71.9ns ± 0% 71.7ns ± 1% -0.39% (p=0.006 n=12+18) RegexpMatchEasy1_1K-12 358ns ± 1% 354ns ± 1% -1.13% (p=0.000 n=20+19) RegexpMatchMedium_32-12 105ns ± 2% 105ns ± 1% -0.63% (p=0.007 n=19+20) RegexpMatchMedium_1K-12 31.9µs ± 1% 31.9µs ± 1% ~ (p=1.000 n=17+17) RegexpMatchHard_32-12 1.51µs ± 1% 1.52µs ± 2% +0.46% (p=0.042 n=18+18) RegexpMatchHard_1K-12 45.3µs ± 1% 45.5µs ± 2% +0.44% (p=0.029 n=18+19) Revcomp-12 388ms ± 1% 385ms ± 0% -0.57% (p=0.000 n=19+18) Template-12 63.0ms ± 1% 63.3ms ± 0% +0.50% (p=0.000 n=19+20) TimeParse-12 309ns ± 1% 307ns ± 0% -0.62% (p=0.000 n=20+20) TimeFormat-12 328ns ± 0% 333ns ± 0% +1.35% (p=0.000 n=19+19) [Geo mean] 47.0µs 46.9µs -0.20% (https://perf.golang.org/search?q=upload:20180326.1) For #10958. For #24543. Change-Id: Icbd52e711fdbe7938a1fea3e6baca1104b53ac3a Reviewed-on: https://go-review.googlesource.com/102604 Run-TryBot: Austin Clements TryBot-Result: Gobot Gobot Reviewed-by: Keith Randall Reviewed-by: David Chase --- src/cmd/compile/internal/gc/fmt.go | 4 +++ src/cmd/compile/internal/gc/range.go | 38 +++++++++++---------- src/cmd/compile/internal/gc/ssa.go | 43 ++++++++++++------------ src/cmd/compile/internal/gc/syntax.go | 29 ++++++++++------ src/cmd/compile/internal/gc/typecheck.go | 3 ++ src/cmd/compile/internal/gc/walk.go | 3 ++ test/prove.go | 28 +++++++++++++++ 7 files changed, 100 insertions(+), 48 deletions(-) diff --git a/src/cmd/compile/internal/gc/fmt.go b/src/cmd/compile/internal/gc/fmt.go index 7df2242226..4e92f5421b 100644 --- a/src/cmd/compile/internal/gc/fmt.go +++ b/src/cmd/compile/internal/gc/fmt.go @@ -994,6 +994,10 @@ func (n *Node) stmtfmt(s fmt.State, mode fmtMode) { fmt.Fprint(s, ";") } + if n.Op == OFORUNTIL && n.List.Len() != 0 { + mode.Fprintf(s, "; %v", n.List) + } + mode.Fprintf(s, " { %v }", n.Nbody) case ORANGE: diff --git a/src/cmd/compile/internal/gc/range.go b/src/cmd/compile/internal/gc/range.go index af818f6f4c..591bd06368 100644 --- a/src/cmd/compile/internal/gc/range.go +++ b/src/cmd/compile/internal/gc/range.go @@ -6,7 +6,6 @@ package gc import ( "cmd/compile/internal/types" - "cmd/internal/objabi" "cmd/internal/sys" "unicode/utf8" ) @@ -254,13 +253,21 @@ func walkrange(n *Node) *Node { break } - if objabi.Preemptibleloops_enabled != 0 { - // Doing this transformation makes a bounds check removal less trivial; see #20711 - // TODO enhance the preemption check insertion so that this transformation is not necessary. - ifGuard = nod(OIF, nil, nil) - ifGuard.Left = nod(OLT, hv1, hn) - translatedLoopOp = OFORUNTIL - } + // TODO(austin): OFORUNTIL is a strange beast, but is + // necessary for expressing the control flow we need + // while also making "break" and "continue" work. It + // would be nice to just lower ORANGE during SSA, but + // racewalk needs to see many of the operations + // involved in ORANGE's implementation. If racewalk + // moves into SSA, consider moving ORANGE into SSA and + // eliminating OFORUNTIL. + + // TODO(austin): OFORUNTIL inhibits bounds-check + // elimination on the index variable (see #20711). + // Enhance the prove pass to understand this. + ifGuard = nod(OIF, nil, nil) + ifGuard.Left = nod(OLT, hv1, hn) + translatedLoopOp = OFORUNTIL hp := temp(types.NewPtr(n.Type.Elem())) tmp := nod(OINDEX, ha, nodintconst(0)) @@ -274,14 +281,11 @@ func walkrange(n *Node) *Node { a.Rlist.Set2(hv1, nod(OIND, hp, nil)) body = append(body, a) - // Advance pointer as part of increment. - // We used to advance the pointer before executing the loop body, - // but doing so would make the pointer point past the end of the - // array during the final iteration, possibly causing another unrelated - // piece of memory not to be garbage collected until the loop finished. - // Advancing during the increment ensures that the pointer p only points - // pass the end of the array during the final "p++; i++; if(i >= len(x)) break;", - // after which p is dead, so it cannot confuse the collector. + // Advance pointer as part of the late increment. + // + // This runs *after* the condition check, so we know + // advancing the pointer is safe and won't go past the + // end of the allocation. tmp = nod(OADD, hp, nodintconst(t.Elem().Width)) tmp.Type = hp.Type @@ -290,7 +294,7 @@ func walkrange(n *Node) *Node { tmp.Right.SetTypecheck(1) a = nod(OAS, hp, tmp) a = typecheck(a, Etop) - n.Right.Ninit.Set1(a) + n.List.Set1(a) case TMAP: // orderstmt allocated the iterator for us. diff --git a/src/cmd/compile/internal/gc/ssa.go b/src/cmd/compile/internal/gc/ssa.go index 7d879395a6..761066ee54 100644 --- a/src/cmd/compile/internal/gc/ssa.go +++ b/src/cmd/compile/internal/gc/ssa.go @@ -971,8 +971,10 @@ func (s *state) stmt(n *Node) { case OFOR, OFORUNTIL: // OFOR: for Ninit; Left; Right { Nbody } - // For = cond; body; incr - // Foruntil = body; incr; cond + // cond (Left); body (Nbody); incr (Right) + // + // OFORUNTIL: for Ninit; Left; Right; List { Nbody } + // => body: { Nbody }; incr: Right; if Left { lateincr: List; goto body }; end: bCond := s.f.NewBlock(ssa.BlockPlain) bBody := s.f.NewBlock(ssa.BlockPlain) bIncr := s.f.NewBlock(ssa.BlockPlain) @@ -1025,30 +1027,29 @@ func (s *state) stmt(n *Node) { b.AddEdgeTo(bIncr) } - // generate incr + // generate incr (and, for OFORUNTIL, condition) s.startBlock(bIncr) if n.Right != nil { s.stmt(n.Right) } - if b := s.endBlock(); b != nil { - b.AddEdgeTo(bCond) - // It can happen that bIncr ends in a block containing only VARKILL, - // and that muddles the debugging experience. - if n.Op != OFORUNTIL && b.Pos == src.NoXPos { - b.Pos = bCond.Pos - } - } - - if n.Op == OFORUNTIL { - // generate code to test condition - s.startBlock(bCond) - if n.Left != nil { - s.condBranch(n.Left, bBody, bEnd, 1) - } else { - b := s.endBlock() - b.Kind = ssa.BlockPlain - b.AddEdgeTo(bBody) + if n.Op == OFOR { + if b := s.endBlock(); b != nil { + b.AddEdgeTo(bCond) + // It can happen that bIncr ends in a block containing only VARKILL, + // and that muddles the debugging experience. + if n.Op != OFORUNTIL && b.Pos == src.NoXPos { + b.Pos = bCond.Pos + } } + } else { + // bCond is unused in OFORUNTIL, so repurpose it. + bLateIncr := bCond + // test condition + s.condBranch(n.Left, bLateIncr, bEnd, 1) + // generate late increment + s.startBlock(bLateIncr) + s.stmtList(n.List) + s.endBlock().AddEdgeTo(bBody) } s.startBlock(bEnd) diff --git a/src/cmd/compile/internal/gc/syntax.go b/src/cmd/compile/internal/gc/syntax.go index 25f421883a..df23b83f29 100644 --- a/src/cmd/compile/internal/gc/syntax.go +++ b/src/cmd/compile/internal/gc/syntax.go @@ -700,16 +700,25 @@ const ( OEMPTY // no-op (empty statement) OFALL // fallthrough OFOR // for Ninit; Left; Right { Nbody } - OFORUNTIL // for Ninit; Left; Right { Nbody } ; test applied after executing body, not before - OGOTO // goto Left - OIF // if Ninit; Left { Nbody } else { Rlist } - OLABEL // Left: - OPROC // go Left (Left must be call) - ORANGE // for List = range Right { Nbody } - ORETURN // return List - OSELECT // select { List } (List is list of OXCASE or OCASE) - OSWITCH // switch Ninit; Left { List } (List is a list of OXCASE or OCASE) - OTYPESW // Left = Right.(type) (appears as .Left of OSWITCH) + // OFORUNTIL is like OFOR, but the test (Left) is applied after the body: + // Ninit + // top: { Nbody } // Execute the body at least once + // cont: Right + // if Left { // And then test the loop condition + // List // Before looping to top, execute List + // goto top + // } + // OFORUNTIL is created by walk. There's no way to write this in Go code. + OFORUNTIL + OGOTO // goto Left + OIF // if Ninit; Left { Nbody } else { Rlist } + OLABEL // Left: + OPROC // go Left (Left must be call) + ORANGE // for List = range Right { Nbody } + ORETURN // return List + OSELECT // select { List } (List is list of OXCASE or OCASE) + OSWITCH // switch Ninit; Left { List } (List is a list of OXCASE or OCASE) + OTYPESW // Left = Right.(type) (appears as .Left of OSWITCH) // types OTCHAN // chan int diff --git a/src/cmd/compile/internal/gc/typecheck.go b/src/cmd/compile/internal/gc/typecheck.go index 6bb41639ee..483be32d6e 100644 --- a/src/cmd/compile/internal/gc/typecheck.go +++ b/src/cmd/compile/internal/gc/typecheck.go @@ -2010,6 +2010,9 @@ func typecheck1(n *Node, top int) *Node { } } n.Right = typecheck(n.Right, Etop) + if n.Op == OFORUNTIL { + typecheckslice(n.List.Slice(), Etop) + } typecheckslice(n.Nbody.Slice(), Etop) decldepth-- diff --git a/src/cmd/compile/internal/gc/walk.go b/src/cmd/compile/internal/gc/walk.go index 257e84cc95..331aefb5de 100644 --- a/src/cmd/compile/internal/gc/walk.go +++ b/src/cmd/compile/internal/gc/walk.go @@ -276,6 +276,9 @@ func walkstmt(n *Node) *Node { } n.Right = walkstmt(n.Right) + if n.Op == OFORUNTIL { + walkstmtlist(n.List.Slice()) + } walkstmtlist(n.Nbody.Slice()) case OIF: diff --git a/test/prove.go b/test/prove.go index 1838bdfd86..9de7d1b3fc 100644 --- a/test/prove.go +++ b/test/prove.go @@ -662,6 +662,34 @@ func oforuntil(b []int) { } } +// The range tests below test the index variable of range loops. + +// range1 compiles to the "efficiently indexable" form of a range loop. +func range1(b []int) { + for i, v := range b { // ERROR "Induction variable: limits \[0,\?\), increment 1$" + b[i] = v + 1 // ERROR "Proved IsInBounds$" + if i < len(b) { // ERROR "Proved Less64$" + println("x") + } + if i >= 0 { // ERROR "Proved Geq64$" + println("x") + } + } +} + +// range2 elements are larger, so they use the general form of a range loop. +func range2(b [][32]int) { + for i, v := range b { + b[i][0] = v[0] + 1 // ERROR "Induction variable: limits \[0,\?\), increment 1$" "Proved IsInBounds$" + if i < len(b) { // ERROR "Proved Less64$" + println("x") + } + if i >= 0 { // ERROR "Proved Geq64" + println("x") + } + } +} + //go:noinline func useInt(a int) { }