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: unexpected memequal call in short string comparison #24765
Comments
In case someone wants to look into this, the relevant bit of code to soup up is from walk.go, // Rewrite comparisons to short constant strings as length+byte-wise comparisons.
var cs, ncs *Node // const string, non-const string
switch {
case Isconst(n.Left, CTSTR) && Isconst(n.Right, CTSTR):
// ignore; will be constant evaluated
case Isconst(n.Left, CTSTR):
cs = n.Left
ncs = n.Right
case Isconst(n.Right, CTSTR):
cs = n.Right
ncs = n.Left
} This code tries to detect strings with known length by looking for constant strings. Fixing this issue would involve adding detection of strings converted from byte (and rune) slices of known length. Might (maybe) be a decent starter compiler issue. |
I’m continuing to try to get started (or maybe continually trying) so I’d like to have a look at this. |
It might make sense to do this via SSA rewrite rules. See the memmove rewrite rules for reference. Without some digging, I couldn't say offhand which is best. One argument for putting it somewhere suitable in the front-end is that this code should boil down to returning a constant: package p
func f() int {
var a [4]byte
return len(string(a[:]))
} |
@ChrisALiles are you working on this? I've encountered this pattern in internal code, so I was thinking on implementing this optimization. |
Yes, after some struggle, I have a prototype fix working and I was planning
to spend the next week or two expanding it.
However, I’m happy to leave it to you if you want it. It’s been a very
educational experience for me and that’s the main thing.
…On Fri, 29 Jun 2018 at 7:22 am, Ilya Tocar ***@***.***> wrote:
@ChrisALiles <https://github.com/ChrisALiles> are you working on this?
I've encountered this pattern in internal code, so I was thinking on
implementing this optimization.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24765 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AeohTsScMDorv55Jnee_EbANIXBdDJXaks5uBUkMgaJpZM4TLsBG>
.
|
@ChrisALiles are you using walk.go or ssa rules based approach? I've implemented something to see if it will speed-up internal code and going to mail it for trybot testing on other architectures, but please continue working on your prototype, as there is plenty of time left until code unfreeze. |
Change https://golang.org/cl/121697 mentions this issue: |
I have a small fix in walk to address Josharian’s “len” example but the
rest is in ssa rules.
On a first look, your change is much better (simpler and more compact) than
what I’ve come up with.
…On Sat, 30 Jun 2018 at 6:29 am, GopherBot ***@***.***> wrote:
Change https://golang.org/cl/121697 mentions this issue: cmd/compile:
inline runtime.memequal if possible
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#24765 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AeohTmWuG0HUbae6je_MuHSM_5JfBKQcks5uBo4lgaJpZM4TLsBG>
.
|
Change https://golang.org/cl/130255 mentions this issue: |
CL 130255 is probably only of academic interest at best seeing that CL 121697 has already been submitted, but I thought I would send it anyway because it cost me a fair amount of effort. A couple of notes: 1. I originally tried to find a way to fix this in walk and found a way to handle Josharian's "len" example above, so I left that in. 2. Don't send it to the bots - it fails in test/codegen because I wasn't brave/silly enough to try anything with architectures other than amd64/386. |
@ChrisALiles CL 121697 is not submitted, due to trybot failures. My understanding is that main problem is that replacing call to memequal with store to the stack may change stack layout and cause write after stack end. I didn't had time to fix that. |
For calls to runtime.memequal with known size, we can just generate single compare instruction. Do this on all architectures, that have appropriate sized unaligned load and compare instructions. Shaves ~8kb from go tool: go_old 12092977 /localdisk/itocar/golang/bin/go 12080689 [-12288 bytes] section differences global text (code) = -5904 bytes (-0.134493%) read-only data = -2839 bytes (-0.098694%) Total difference -8743 bytes (-0.112717%) Fixes golang#24765 Change-Id: I13fcb7ac046df6915521340ec6321465c7f4e93c
(Using Go tip, 8818b4d)
Compiling this code on amd64,
I would expect that to compile to something pretty minimal, since the memory comparison must always be 4 bytes, but instead I see a call to memequal:
Ideally we'd just do a 4 byte (potentially unaligned) load from the string into a register and compare with the receiver?
/cc @josharian @randall77 @rasky
The text was updated successfully, but these errors were encountered: