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: odd bounds check elimination #25537

Closed
zhengxiaoyao0716 opened this issue May 24, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@zhengxiaoyao0716
Copy link

commented May 24, 2018

What version of Go are you using (go version)?

image

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

image

image

What did you do?

package main

// Closure .
func Closure(as []interface{}, bs []interface{}) {
	_ = func() {
		_ = as[:len(as)] // [L6:9] checked?
		_ = bs[:len(bs)] // [L7:9] checked?
		// swap
		_ = as[:len(bs)] // checked, that's right.
		_ = bs[:len(as)] // checked, that's right.
	}
}

// NoClosure .
func NoClosure(as []interface{}, bs []interface{}) {
	_ = as[:len(as)] // eliminatd, that's right.
	_ = bs[:len(bs)] // eliminatd, that's right.
	// swap
	_ = as[:len(bs)] // checked, that's right.
	_ = bs[:len(as)] // checked, that's right.
}

// ClosureAssert .
func ClosureAssert(as []interface{}, bs []interface{}) {
	_ = func() {
		if len(as) == len(bs) {
			_ = as[:len(bs)] // [L27] eliminatd, why?
			_ = bs[:len(as)] // [L28] eliminatd, why?
			_ = as[:len(as)] // [L29:10] checked, why?
			_ = bs[:len(bs)] // [L30:10] checked, why?
		}
	}
}

func main() {}
go build -gcflags="-d=ssa/check_bce/debug=1" main.go

What did you expect to see?

See the comments of the code above.

What did you see instead?

image

@ianlancetaylor ianlancetaylor changed the title Some strange performance for BCE check. cmd/compile: odd bounds check elimination May 24, 2018

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone May 24, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 24, 2018

@aclements

This comment has been minimized.

Copy link
Member

commented May 24, 2018

Hi @zhengxiaoyao0716. You may be happy to know that this will improve significantly in Go 1.11, which can eliminate the two bounds checks in Closure and all four bounds checks in ClosureAssert.

In Go 1.10, you happen to be hitting some of the weirder corner cases of the compiler.

In Closure, the references to the captured variables get rewritten into pointer dereferences. Early on, the compiler applies a simple rule that matches exactly the as[:len(as)] pattern, but because of the extra level of indirection for captured variables, the pattern doesn't match. It does match in NoClosure because as is a local variable, not a captured variable.

ClosureAssert is even weirder, I'm afraid. Just like Closure, that doesn't trigger the simple as[:len(as)] pattern, but it does trigger the more sophisticated "prove" pass, which understands the general relationships between slice lengths and capacities. However, because of the structure of "prove" in Go 1.10, the IsSliceInBounds alone (like in Closure) wasn't sufficient to trigger the fact that len(x) <= cap(x). But the equality comparison added in ClosureAssert was enough to trigger this fact, and once that fact was available it could use it for the IsSliceInBounds. That's why this works in ClosureAssert but not Closure. We fixed this in commit 669db2c.

Hopefully that explains things. Since this will be fixed in Go 1.11, I'm closing the issue.

@bradfitz bradfitz closed this May 24, 2018

@zhengxiaoyao0716

This comment has been minimized.

Copy link
Author

commented May 25, 2018

@aclements Thanks for your patient answer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.