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: remove bounds checking for sub-slices #14905

Closed
OneOfOne opened this issue Mar 22, 2016 · 6 comments
Closed

cmd/compile: remove bounds checking for sub-slices #14905

OneOfOne opened this issue Mar 22, 2016 · 6 comments
Labels
FrozenDueToAge
Milestone
Unplanned

Comments

@OneOfOne
Copy link
Contributor

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version devel +259b7ed 2016-03-22 00:18:31 +0000 linux/amd64
  2. What operating system and processor architecture are you using (go env)?
    Linux Ava 4.4.5-1-ARCH #1 SMP PREEMPT Thu Mar 10 07:38:19 CET 2016 x86_64 GNU/Linux
  3. What did you do?
func a(in []byte) uint64 {
    return uint64(in[0]) | uint64(in[1])<<8 | uint64(in[2])<<16 | uint64(in[3])<<24 |
        uint64(in[4])<<32 | uint64(in[5])<<40 | uint64(in[6])<<48 | uint64(in[7])<<56
}

func b(p []byte) uint64 {
        p = p[:16]
    return a(p[8:8])
}
  1. What did you expect to see?
    No check for p[8:8]
  2. What did you see instead?
        0x001a 00026 (blah.go:10)       CMPQ    CX, $8
        0x001e 00030 (blah.go:10)       JCS     $0, 39

On a side note, by inlining that function by hand, some of my code is now faster than using unsafe, so there's that.

Amazing job with the SSA branch guys.
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Mar 22, 2016
@ianlancetaylor
Copy link
Member

CC @randall77 @dr2chase

@randall77
Copy link
Contributor

I think you mean p[8:16], not p[8:8]. p[8:8] is guaranteed to panic when given to a.

But I think your general issue is still there. p[8:16] looks ok, but p[7:15] has the same extra comparison in there that p[8:8] has. After we do cap(p)>=16, we know to get rid of another cap(p)>=16, but not a cap(p)>=15.

@brtzsnr

@brtzsnr
Copy link
Contributor

brtzsnr commented Mar 22, 2016

It should be p = p[:16:len(p)] otherwise you extend p over the original length. This is binary.BigEndian.Uint64(), right? Why not use that instead, we might be able to do something faster than your example.

@brtzsnr
Copy link
Contributor

brtzsnr commented Mar 22, 2016

There is some more opportunity here to optimize:

    v9 = Const64 <int> [16]
    v8 = SliceLen <int> v7
    v17 = IsSliceInBounds <bool> v9 v8

So we have v8 >= v9. Later

    v25 = Const64 <int> [8]
    v35 = Eq64 <bool> v25 v8

v35 cannot be true. Related to #14900.

gopherbot pushed a commit that referenced this issue Mar 22, 2016
@brtzsnr
cmd/compile: simplify SliceCap when it equals SliceLen
e61db31 
Shows up occassionally, especially after p = p[:8:len(p)]

Updates #14905

Change-Id: Iab35ef2eac57817e6a10c6aaeeb84709e8021641
Reviewed-on: https://go-review.googlesource.com/21025
Run-TryBot: Alexandru Moșoi <alexandru@mosoi.ro>
Reviewed-by: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@brtzsnr
Copy link
Contributor

brtzsnr commented Apr 1, 2016

I think this is fixed after https://go-review.googlesource.com/#/c/21008/

@OneOfOne
Copy link
Contributor Author

OneOfOne commented Apr 2, 2016

Yep, I just confirmed, closing.

@OneOfOne OneOfOne closed this as completed Apr 2, 2016
@golang golang locked and limited conversation to collaborators Apr 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@brtzsnr @OneOfOne @ianlancetaylor @randall77 @gopherbot