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: bounds check not needed #61333

Open
KaurkerDevourer opened this issue Jul 13, 2023 · 4 comments
Open

cmd/compile: bounds check not needed #61333

KaurkerDevourer opened this issue Jul 13, 2023 · 4 comments
Labels
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
Unplanned

Comments

@KaurkerDevourer
Copy link

KaurkerDevourer commented Jul 13, 2023 •
edited by randall77

https://godbolt.org/z/EW16jnEG7
As you can see, there is panicIndex, on s[I + j]. But in golang, for i := 0; I < len(s); I++ doesn't go into panicIndex. Why does

for i := 0; i < len(s) - len(t); i++ {
    for j := 0; j < len(t); j++ {
         s[i + j]
    }
}

calls panicIndex? There is no need in this. Compiler sees that i + j is always in range [0, len(s)). Easy optimisation, probably not only in this case.
@gopherbot gopherbot added this to the Proposal milestone Jul 13, 2023
@earthboundkid
Copy link
Contributor

This doesn’t need to be a proposal. If you can write the optimization for the compiler and prove it works, that’s sufficient.

@KaurkerDevourer
Copy link
Author

This doesn’t need to be a proposal. If you can write the optimization for the compiler and prove it works, that’s sufficient.

Hm, okey =)
Let me try this on a weekend. Can you please help me, im newbie in golang core code - where can I find anything about panicIndex and its optimisation (It is not callable in for I = 0; I < len(s); I++ f.e.)?
If anyone has an idea, help me with this please.

@randall77 randall77 changed the title proposal: panicIndex sometimes not needed cmd/compile: bounds check not needed Jul 13, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2023
@randall77
Copy link
Contributor

Optimizations like this are generally done in the prove pass, in cmd/compile/internal/ssa/prove.go.
Modifying that code is not for the feint of heart. There are lots of tricky details like getting the behavior on overflow/underflow correct.

@randall77 randall77 modified the milestones: Proposal, Unplanned Jul 13, 2023
@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 14, 2023
@KaurkerDevourer
Copy link
Author

I have candidates for what I need, but im not sure.
https://github.com/golang/go/blob/master/src/cmd/compile/internal/ssa/prove.go#L800
and
https://github.com/golang/go/blob/master/src/cmd/compile/internal/ssa/writebarrier.go
But where can I find any docs about this exact files. Comments do not provide needed information. Maybe someone just knows and can point me at the code that removes redundant panicIndex?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
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
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@earthboundkid @ianlancetaylor @randall77 @gopherbot @cherrymui @KaurkerDevourer