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

Jump to bottom

cmd/compile: odd bounds check elimination #25537

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

cmd/compile: odd bounds check elimination #25537

zhengxiaoyao0716 opened this issue May 24, 2018 · 3 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.11

Comments

@zhengxiaoyao0716
Copy link

zhengxiaoyao0716 commented May 24, 2018 •
edited

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 24, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone May 24, 2018
@ianlancetaylor
Copy link
Contributor

CC @randall77 @aclements

@aclements
Copy link
Member

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.

@zhengxiaoyao0716
Copy link
Author

@aclements Thanks for your patient answer!

@golang golang locked and limited conversation to collaborators May 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
5 participants
@bradfitz @aclements @ianlancetaylor @gopherbot @zhengxiaoyao0716