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: unnecessary bounds check when indexing slice from range #71439

Closed
dsnet opened this issue Jan 26, 2025 · 5 comments
Closed

cmd/compile: unnecessary bounds check when indexing slice from range #71439

dsnet opened this issue Jan 26, 2025 · 5 comments
Labels
BugReport Issues describing a possible bug in the Go implementation. compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Jan 26, 2025

Go version

go1.23

Output of go env in your module/workspace:

GOARCH=amd64
GOOS=linux

What did you do?

Compile the following:

package main

import "archive/tar"

var src []tar.Header
var dst *tar.Header

func main() {
	for i := range src {
		dst = &src[i] // line 10
	}
}

What did you see happen?

I see this compiled out:

...
0x0069 00105 (main.go:10)	PCDATA	$1, $0
0x0069 00105 (main.go:10)	CALL	runtime.panicIndex(SB)
0x006e 00110 (main.go:10)	XCHGL	AX, AX
...

What did you expect to see?

No such call to runtime.panicIndex. The slice is indexed from an iteration integer that is provably bounded by the length of src itself. The only possible way a panic occurs is if src is mutated during the iteration (or asynchronously in another goroutine, in which case there must be synchronization primitives).

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jan 26, 2025
@dsnet
Copy link
Member Author

dsnet commented Jan 26, 2025

Oh interesting, the following does not emit a call to runtime.panicIndex:

for i := 0; i < len(src); i++ {
	dst = &src[i]
}

which I assume is because i < len(src) is evaluated on every iteration.

@randall77
Copy link
Contributor

The compiler generally gives up on analyzing globals. Load from the global to a local before the loop and it should be fine.

I think the difference between your two examples is in the first, src is read once outside the loop and once inside the loop. Those two reads are not CSEd. In the second example, the two loads of src are both inside the loop so they get CSEd. That lets the bounds check removal logic see that the bounds check is unnecessary.

@gabyhelp gabyhelp added the BugReport Issues describing a possible bug in the Go implementation. label Jan 26, 2025
@zigo101
Copy link

zigo101 commented Jan 26, 2025

#47736

@cagedmantis cagedmantis added this to the Backlog milestone Jan 27, 2025
@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 27, 2025
@mknyszek mknyszek modified the milestones: Backlog, Unplanned Feb 5, 2025
@mknyszek
Copy link
Contributor

mknyszek commented Feb 5, 2025

In triage, @randall77 notes there isn't really much to be done here, so closing for now. Thanks.

@mknyszek mknyszek closed this as not planned Won't fix, can't repro, duplicate, stale Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BugReport Issues describing a possible bug in the Go implementation. compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
Development

No branches or pull requests

7 participants