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.

Sign up for GitHub

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

Jump to bottom

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
@randall77
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. release-blocker
Milestone
Go1.21

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

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

@cuonglm cuonglm mentioned this issue Jun 26, 2023
Closed
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
@randall77 @bradfitz
cmd/compile: fix min/max builtin code generation
e412050 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@randall77 randall77

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. release-blocker
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
2 participants
@randall77 @gopherbot