Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: miscompilation of min/max builtin #60982

Closed
randall77 opened this issue Jun 23, 2023 · 1 comment
Closed

cmd/compile: miscompilation of min/max builtin #60982

randall77 opened this issue Jun 23, 2023 · 1 comment
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge release-blocker
Milestone

Comments

@randall77
Copy link
Contributor

package main

func f(x int) int {
	if x >= 1000 {
		return max(x, 2000)
	}
	switch x {
	case 0:
		return 0
	case 1:
		return 1
	case 2:
		return 2
... 996 more cases ...
	case 999:
		return 999
	}
	return 0
}

This program causes the compiler to crash on tip. The problem stems from an incompatibility between how max is implemented and the phi placement algorithm we use for large functions (hence the need for all the switch cases to reproduce it).

@randall77 randall77 added this to the Go1.21 milestone Jun 23, 2023
@randall77 randall77 self-assigned this Jun 23, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jun 23, 2023
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/505756 mentions this issue: cmd/compile: fix min/max builtin code generation

bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
Our large-function phi placement algorithm is incompatible with phi
opcodes already existing in the SSA representation. Instead, use simple
variable assignments and have the phi placement algorithm place the phis
we need for min/max.

Turns out the small-function phi placement algorithm doesn't have this
sensitivity, so this bug only occurs in large functions (>500 basic blocks).

Maybe we should document/check that no phis are present when we start
phi placement (regardless of size).  Leaving for a potential separate CL.

We should probably also fix the placement algorithm to handle existing
phis correctly.  But this CL is probably a lot smaller/safer than
messing with phi placement.

Fixes golang#60982

Change-Id: I59ba7f506c72b22bc1485099a335d96315ebef67
Reviewed-on: https://go-review.googlesource.com/c/go/+/505756
Reviewed-by: Keith Randall <khr@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
@golang golang locked and limited conversation to collaborators Jun 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge release-blocker
Projects
None yet
Development

No branches or pull requests

2 participants