Remove runtime.isnil #990
Merged
Remove runtime.isnil #990
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This gives a hint to the compiler that such parameters are either NULL or point to a valid object that can be dereferenced. This is not directly very useful, but is very useful when combined with https://reviews.llvm.org/D60047 to remove the runtime.isnil hack without regressing escape analysis.
The x/tools/go/ssa package splits slice loads/stores into two
operations. So for code like this:
x = p[3]
It has two instructions:
x_ptr = &p[3]
x = *x_ptr
This makes the IR simpler, but also means we're accidentally inserting
more nil checks than necessary: the slice index operation has
effectively already checked for nil by performing a bounds check.
Therefore, omit nil pointer checks for pointers created by
*ssa.IndexAddr.
This change is necessary to make sure a future removal of runtime.isnil
will not cause the escape analysis pass to regress. Apart from that, it
reduces code size slightly in many smoke tests (with no increases in
code size).
The unsafe.Pointer type is used for many low-level operations, especially in the runtime. It can for example be used to copy the contents of a slice (in the copy builtin) independent of the slice element type.
This hack was originally introduced in #251 to fix an escape analysis regression after #222 introduced nil checks. Since a new optimization in LLVM (see https://reviews.llvm.org/D60047) this hack is not necessary anymore and can be removed. I've compared all regular tests and smoke tests before and after to check the size. In most cases this change was an improvement although there are a few regressions.
Member
|
Now merging, thanks @aykevl |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Finally, I think this branch is ready.
The first three commits are commits to prepare for the change in the fourth commit, which is what this PR is about. They make sure escape analysis won't regress after
runtime.isnilhas been removed.I found exactly one regression in escape analysis: in testdata/calls.go, but only on a host OS and on WebAssembly. Further investigation revealed that it has something to do with function pointers or closures.
Most tests I've done (including smoke tests) result in a reduction of code size however some get bigger (perhaps because the inliner made different choices somewhere). None of the baremetal tests resulted in an escape analysis regression. Overall I think this is an improvement.
(Note that the GitHub ordering of commits is wrong, they're sorted by date and not by actual commit order).