Skip to content

Conversation

@aykevl
Copy link
Member

@aykevl aykevl commented Mar 19, 2020

The x/tools/go/ssa package splits slice loads/stores into two operations. So for code like this:

x = p[3]

It outputs 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 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).
@deadprogram
Copy link
Member

@aykevl this PR now needs a merge conflict resolved. Thanks.

@aykevl
Copy link
Member Author

aykevl commented Mar 26, 2020

Closing in favor of #990, which contains this commit but in a slightly different form (and rebased on top of dev after #746 landed). I can update this PR with that commit if needed if that makes review easier.

@aykevl aykevl closed this Mar 26, 2020
@deadprogram deadprogram deleted the nilcheck-IndexAddr branch January 4, 2022 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants