Skip to content

Commit 788a227

Browse files
committed
cmd/compile: fix SCCP propagation into jump tables
We can't delete all the outgoing edges and then add one back in, because then we've lost the argument of any phi at the target. Instead, move the important target to the front of the list and delete the rest. This normally isn't a problem, because there is never normally a phi at the target of a jump table. But this isn't quite true when in race build mode, because there is a phi of the result of a bunch of raceread calls. The reason this happens is that each case is written like this (where e is the runtime.eface we're switching on): if e.type == $type.int32 { m = raceread(e.data, m1) } m2 = phi(m1, m) if e.type == $type.int32 { .. do case .. goto blah } so that if e.type is not $type.int32, it falls through to the default case. This default case will have a memory phi for all the (jumped around and not actually called) raceread calls. If we instead did it like if e.type == $type.int32 { raceread(e.data) .. do case .. goto blah } That would paper over this bug, as it is the only way to construct a jump table whose target is a block with a phi in it. (Yet.) But we'll fix the underlying bug in this CL. Maybe we can do the rewrite mentioned above later. (It is an optimization for -race mode, which isn't particularly important.) Fixes #64606 Change-Id: I6f6e3c90eb1e2638112920ee2e5b6581cef04ea4 Reviewed-on: https://go-review.googlesource.com/c/go/+/548356 Reviewed-by: Keith Randall <[email protected]> Reviewed-by: David Chase <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
1 parent dca2ef2 commit 788a227

File tree

4 files changed

+54
-5
lines changed

4 files changed

+54
-5
lines changed

src/cmd/compile/internal/ssa/block.go

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -297,6 +297,8 @@ func (b *Block) removePred(i int) {
297297
// removeSucc removes the ith output edge from b.
298298
// It is the responsibility of the caller to remove
299299
// the corresponding predecessor edge.
300+
// Note that this potentially reorders successors of b, so it
301+
// must be used very carefully.
300302
func (b *Block) removeSucc(i int) {
301303
n := len(b.Succs) - 1
302304
if i != n {
@@ -323,6 +325,19 @@ func (b *Block) swapSuccessors() {
323325
b.Likely *= -1
324326
}
325327

328+
// Swaps b.Succs[x] and b.Succs[y].
329+
func (b *Block) swapSuccessorsByIdx(x, y int) {
330+
if x == y {
331+
return
332+
}
333+
ex := b.Succs[x]
334+
ey := b.Succs[y]
335+
b.Succs[x] = ey
336+
b.Succs[y] = ex
337+
ex.b.Preds[ex.i].i = y
338+
ey.b.Preds[ey.i].i = x
339+
}
340+
326341
// removePhiArg removes the ith arg from phi.
327342
// It must be called after calling b.removePred(i) to
328343
// adjust the corresponding phi value of the block:
@@ -339,7 +354,7 @@ func (b *Block) swapSuccessors() {
339354
func (b *Block) removePhiArg(phi *Value, i int) {
340355
n := len(b.Preds)
341356
if numPhiArgs := len(phi.Args); numPhiArgs-1 != n {
342-
b.Fatalf("inconsistent state, num predecessors: %d, num phi args: %d", n, numPhiArgs)
357+
b.Fatalf("inconsistent state for %v, num predecessors: %d, num phi args: %d", phi, n, numPhiArgs)
343358
}
344359
phi.Args[i].Uses--
345360
phi.Args[i] = phi.Args[n]

src/cmd/compile/internal/ssa/deadcode.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,8 @@ func deadcode(f *Func) {
312312

313313
// removeEdge removes the i'th outgoing edge from b (and
314314
// the corresponding incoming edge from b.Succs[i].b).
315+
// Note that this potentially reorders successors of b, so it
316+
// must be used very carefully.
315317
func (b *Block) removeEdge(i int) {
316318
e := b.Succs[i]
317319
c := e.b

src/cmd/compile/internal/ssa/sccp.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -533,12 +533,12 @@ func rewireSuccessor(block *Block, constVal *Value) bool {
533533
block.ResetControls()
534534
return true
535535
case BlockJumpTable:
536+
// Remove everything but the known taken branch.
536537
idx := int(constVal.AuxInt)
537-
targetBlock := block.Succs[idx].b
538-
for len(block.Succs) > 0 {
539-
block.removeEdge(0)
538+
block.swapSuccessorsByIdx(0, idx)
539+
for len(block.Succs) > 1 {
540+
block.removeEdge(1)
540541
}
541-
block.AddEdgeTo(targetBlock)
542542
block.Kind = BlockPlain
543543
block.Likely = BranchUnknown
544544
block.ResetControls()

test/fixedbugs/issue64606.go

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
// build -race
2+
3+
//go:build race
4+
5+
// Copyright 2023 The Go Authors. All rights reserved.
6+
// Use of this source code is governed by a BSD-style
7+
// license that can be found in the LICENSE file.
8+
9+
package main
10+
11+
func main() {
12+
var o any = uint64(5)
13+
switch o.(type) {
14+
case int:
15+
goto ret
16+
case int8:
17+
goto ret
18+
case int16:
19+
goto ret
20+
case int32:
21+
goto ret
22+
case int64:
23+
goto ret
24+
case float32:
25+
goto ret
26+
case float64:
27+
goto ret
28+
default:
29+
goto ret
30+
}
31+
ret:
32+
}

0 commit comments

Comments
 (0)