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: switch statements over constants are not inlined #50253

Closed
zx2c4 opened this issue Dec 18, 2021 · 5 comments
Closed

cmd/compile: switch statements over constants are not inlined #50253

zx2c4 opened this issue Dec 18, 2021 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Dec 18, 2021

It appears that the inliner runs after constants in if statements are evaluated, which is good, but runs before constants in switch statements are evaluated, which is not good.

This is a problem for switch statements over, say, runtime.GOOS or runtime.GOARCH. Here's an example to reproduce the problem:

package main

import "runtime"

func OSNameIf() string {
	if runtime.GOOS == "linux" {
		return "Leenooks"
	} else if runtime.GOOS == "windows" {
		return "Windoze"
	} else if runtime.GOOS == "darwin" {
		return "MackBone"
	} else if runtime.GOOS == "1" || runtime.GOOS == "2" || runtime.GOOS == "3" || runtime.GOOS == "4" || runtime.GOOS == "5" || runtime.GOOS == "6" || runtime.GOOS == "7" || runtime.GOOS == "8" || runtime.GOOS == "9" || runtime.GOOS == "10" || runtime.GOOS == "11" || runtime.GOOS == "12" || runtime.GOOS == "13" || runtime.GOOS == "14" || runtime.GOOS == "15" || runtime.GOOS == "16" || runtime.GOOS == "17" || runtime.GOOS == "18" || runtime.GOOS == "19" || runtime.GOOS == "20" || runtime.GOOS == "21" || runtime.GOOS == "22" || runtime.GOOS == "23" || runtime.GOOS == "24" || runtime.GOOS == "25" || runtime.GOOS == "26" || runtime.GOOS == "27" || runtime.GOOS == "28" || runtime.GOOS == "29" || runtime.GOOS == "30" || runtime.GOOS == "31" || runtime.GOOS == "32" || runtime.GOOS == "33" || runtime.GOOS == "34" || runtime.GOOS == "35" || runtime.GOOS == "36" || runtime.GOOS == "37" || runtime.GOOS == "38" || runtime.GOOS == "39" || runtime.GOOS == "40" || runtime.GOOS == "41" || runtime.GOOS == "42" || runtime.GOOS == "43" || runtime.GOOS == "44" || runtime.GOOS == "45" || runtime.GOOS == "46" || runtime.GOOS == "47" || runtime.GOOS == "48" || runtime.GOOS == "49" || runtime.GOOS == "50" || runtime.GOOS == "51" || runtime.GOOS == "52" || runtime.GOOS == "53" || runtime.GOOS == "54" || runtime.GOOS == "55" || runtime.GOOS == "56" || runtime.GOOS == "57" || runtime.GOOS == "58" || runtime.GOOS == "59" || runtime.GOOS == "60" || runtime.GOOS == "61" || runtime.GOOS == "62" || runtime.GOOS == "63" || runtime.GOOS == "64" || runtime.GOOS == "65" || runtime.GOOS == "66" || runtime.GOOS == "67" || runtime.GOOS == "68" || runtime.GOOS == "69" || runtime.GOOS == "70" || runtime.GOOS == "71" || runtime.GOOS == "72" || runtime.GOOS == "73" || runtime.GOOS == "74" || runtime.GOOS == "75" || runtime.GOOS == "76" || runtime.GOOS == "77" || runtime.GOOS == "78" || runtime.GOOS == "79" || runtime.GOOS == "80" || runtime.GOOS == "81" || runtime.GOOS == "82" || runtime.GOOS == "83" || runtime.GOOS == "84" || runtime.GOOS == "85" || runtime.GOOS == "86" || runtime.GOOS == "87" || runtime.GOOS == "88" || runtime.GOOS == "89" || runtime.GOOS == "90" || runtime.GOOS == "91" || runtime.GOOS == "92" || runtime.GOOS == "93" || runtime.GOOS == "94" || runtime.GOOS == "95" || runtime.GOOS == "96" || runtime.GOOS == "97" || runtime.GOOS == "98" || runtime.GOOS == "99" || runtime.GOOS == "100" {
		return "Numbers"
	} else {
		panic("oh nose!")
	}
}

func OSNameSwitch() string {
	switch runtime.GOOS {
	case "linux":
		return "Leenooks"
	case "windows":
		return "Windoze"
	case "darwin":
		return "MackBone"
	case "1", "2", "3", "4", "5", "6", "7", "8", "9", "10", "11", "12", "13", "14", "15", "16", "17", "18", "19", "20", "21", "22", "23", "24", "25", "26", "27", "28", "29", "30", "31", "32", "33", "34", "35", "36", "37", "38", "39", "40", "41", "42", "43", "44", "45", "46", "47", "48", "49", "50", "51", "52", "53", "54", "55", "56", "57", "58", "59", "60", "61", "62", "63", "64", "65", "66", "67", "68", "69", "70", "71", "72", "73", "74", "75", "76", "77", "78", "79", "80", "81", "82", "83", "84", "85", "86", "87", "88", "89", "90", "91", "92", "93", "94", "95", "96", "97", "98", "99", "100":
		return "Numbers"
	default:
		panic("oh nose!")
	}
}

func main() {
	println(OSNameIf())
	println(OSNameSwitch())
}
$ go run -gcflags=-m=2 test.go 
# command-line-arguments
./test.go:5:6: can inline OSNameIf with cost 2 as: func() string { if runtime.GOOS == "linux" { return "Leenooks" } }
./test.go:18:6: cannot inline OSNameSwitch: function too complex: cost 121 exceeds budget 80
./test.go:33:6: can inline main with cost 65 as: func() { println(OSNameIf()); println(OSNameSwitch()) }
./test.go:34:18: inlining call to OSNameIf func() string { if runtime.GOOS == "linux" { return "Leenooks" } }
./test.go:29:8: "oh nose!" escapes to heap:
./test.go:29:8:   flow: {heap} = &{storage for "oh nose!"}:
./test.go:29:8:     from "oh nose!" (spill) at ./test.go:29:8
./test.go:29:8:     from panic("oh nose!") (call parameter) at ./test.go:29:8
./test.go:29:8: "oh nose!" escapes to heap
Leenooks
Leenooks

The switch statement doesn't get inlined, because it's too big, while the if statement does, because it early on reduces to a cost of two.

And confirmed by the assembly listing:

image

CC @cuonglm @josharian

@ALTree ALTree added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance labels Dec 18, 2021
@josharian
Copy link
Contributor

josharian commented Dec 18, 2021

We should definitely be able to get at least the most obvious/trivial of constant-foldable switch statements, which will probably handle the most performance-sensitive cases, like runtime.GOOS and runtime.GOARCH.

Cuong, is this something you'd like to tackle?

@cuonglm
Copy link
Member

cuonglm commented Dec 18, 2021

We should definitely be able to get at least the most obvious/trivial of constant-foldable switch statements, which will probably handle the most performance-sensitive cases, like runtime.GOOS and runtime.GOARCH.

Cuong, is this something you'd like to tackle?

I will take a look once the freeze is over.

@quasilyte
Copy link
Contributor

quasilyte commented Dec 23, 2021

Some stats from the 11M SLOC code corpus.

  • switch runtime.GOOS { $*_ } - 288 matches
  • switch runtime.GOARCH { $*_ } - 33 matches

GOROOT-only results:

  • switch runtime.GOOS { $*_ } - 156 matches (1 match per ~7200 SLOC)
  • switch runtime.GOARCH { $*_ } - 26 matches

@gopherbot
Copy link

gopherbot commented Jan 29, 2022

Change https://golang.org/cl/381934 mentions this issue: cmd/compile: add constant folding switch statements in deadcode

@gopherbot
Copy link

gopherbot commented Apr 11, 2022

Change https://go.dev/cl/399694 mentions this issue: cmd/compile: constant-fold switches early in compilation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Development

No branches or pull requests

6 participants