transform: elide allocations from byte slice equality tests #5067
Conversation
This change remove the allocation for Go idioms that temporarily convert a bytes slice to a string. For example `string(slice) == "some string"`. In particular, `bytes.Equal` no longer allocates two strings.
aykevl
left a comment
aykevl
left a comment
There was a problem hiding this comment.
It's hard for me to confirm this is indeed correct, I find it hard to understand what's happening here. Perhaps some refactoring would help? Or can you give a slightly higher-level overview of what this optimization does exactly (and why it is correct)?
For example, can you help me understand how the following code for example skips this optimization?
slice := []byte("foo")
str := string(slice)
if true { // new basic block
slice[0] = 'A'
return str == "foo"
}There was a problem hiding this comment.
It would be really nice to have some tests that are easier to read than LLVM IR. Could you add some tests like transform/testdata/allocs2.go? (This will probably require some refactoring to not duplicate code). This will also show that the optimization works for real Go code, instead of handcrafted IR (that might go out of date).
| stringEqual := mod.NamedFunction("runtime.stringEqual") | ||
| if stringEqual.IsNil() { | ||
| return | ||
| } |
There was a problem hiding this comment.
This could be generalized to any function:
- That has a function attribute like
memory(argmem: read),memory(read), etc (in other words, doesn't modify memory). This should be automatically deduced forruntime.stringEqualbut if not we can add it to compiler/symbol.go. - Where the string pointer parameter has the parameter attribute
nocapture(meaning the pointer parameter is not kept across the function call).
For details, see: https://llvm.org/docs/LangRef.html#fnattrs and https://llvm.org/docs/LangRef.html#paramattrs.
| inst = llvm.NextInstruction(inst) | ||
| if inst.IsNil() { | ||
| // There are uses beyond this basic block. | ||
| continue uses |
There was a problem hiding this comment.
I find this to be hard to read. Can you refactor this code to avoid the continue uses? Perhaps by moving some of it to a separate function?
|
Thanks for the review, but I think this is redundant; I confused myself by an unfortunate combination of forgetting |
|
It does optimize stuff in at least some cases: I can't say they are correct, but it says there is something here that could be optimized. |
|
All those builds run with the default |
Not sure what you mean by that? |
|
I mean that according to my experiments, this optimization has no additional effect when I build with |
|
Yes, that could be it. Can't say for sure. But it's often the case that more inlining enables more compiler optimizations since most compiler optimizations work on a per-function basis and not across functions. (In fact, the main goal of inlining is to enable more of these optimizations, the reduced call overhead is just an added benefit). |
2 participants
This change remove the allocation for Go idioms that temporarily convert a bytes slice to a string. For example
string(slice) == "some string". In particular,bytes.Equalno longer allocates two strings.