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: bounds check not removed after "if index < length" #15074

Open
rasky opened this Issue Apr 2, 2016 · 4 comments

Comments

Projects
None yet
3 participants
@rasky
Member

rasky commented Apr 2, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version devel +6a0bb87 Sat Apr 2 02:08:59 2016 +0000 darwin/amd64
  2. What operating system and processor architecture are you using (go env)?
    GOARCH="amd64"
    GOBIN=""
    GOEXE=""
    GOHOSTARCH="amd64"
    GOHOSTOS="darwin"
    GOOS="darwin"
    GOPATH="/Users/rasky/Sources/go"
    GORACE=""
    GOROOT="/usr/local/Cellar/go/1.6/libexec"
    GOTOOLDIR="/usr/local/Cellar/go/1.6/libexec/pkg/tool/darwin_amd64"
    GO15VENDOREXPERIMENT="1"
    CC="clang"
    GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fno-common"
    CXX="clang++"
    CGO_ENABLED="1"
  3. What did you do?
package main

import (
    "os"
    "strconv"
)

var table = [8]uint32{
    1 << 30, 1 << 30,
    1 << 29, 1 << 29,
    1 << 31, 1 << 31,
    1 << 28, 1 << 28,
}

var value uint32

//go:noinline
func Bound1(cond uint32) bool {
    if cond < 8 {
        v := value
        if cond&1 != 0 {
            v = ^v
        }
        return v&table[cond] != 0   // bound check should be removed here
    }
    return false
}

func main() {
    val, _ := strconv.Atoi(os.Args[1])
    Bound1(uint32(val))
}
  1. What did you expect to see?
    Bound check removed
  2. What did you see instead?
TEXT main.Bound1(SB) /Users/rasky/Sources/go/src/ndsemu/bugs/bound1.go
    bound1.go:18    0x2040  65488b0c25a0080000  GS MOVQ GS:0x8a0, CX
    bound1.go:18    0x2049  483b6110        CMPQ 0x10(CX), SP
    bound1.go:18    0x204d  7642            JBE 0x2091
    bound1.go:19    0x204f  8b4c2408        MOVL 0x8(SP), CX
    bound1.go:19    0x2053  83f908          CMPL $0x8, CX
    bound1.go:19    0x2056  7333            JAE 0x208b
    bound1.go:15    0x2058  8b1542aa0b00        MOVL 0xbaa42(IP), DX
    bound1.go:21    0x205e  f7c101000000        TESTL $0x1, CX
    bound1.go:21    0x2064  7402            JE 0x2068
    bound1.go:22    0x2066  f7d2            NOTL DX
    bound1.go:24    0x2068  8bc9            MOVL CX, CX
    bound1.go:24    0x206a  4883f908        CMPQ $0x8, CX
    bound1.go:24    0x206e  7314            JAE 0x2084
    bound1.go:24    0x2070  488d0529e00900      LEAQ 0x9e029(IP), AX
    bound1.go:24    0x2077  8b0c88          MOVL 0(AX)(CX*4), CX
    bound1.go:24    0x207a  85d1            TESTL DX, CX
    bound1.go:24    0x207c  0f95c0          SETNE AL
    bound1.go:24    0x207f  88442410        MOVL AL, 0x10(SP)
    bound1.go:24    0x2083  c3          RET
    bound1.go:24    0x2084  e857e30100      CALL runtime.panicindex(SB)
    bound1.go:24    0x2089  0f0b            UD2
    bound1.go:26    0x208b  c644241000      MOVL $0x0, 0x10(SP)
    bound1.go:26    0x2090  c3          RET
    bound1.go:18    0x2091  e81a1b0400      CALL runtime.morestack_noctxt(SB)
    bound1.go:18    0x2096  eba8            JMP main.Bound1(SB)
    bound1.go:18    0x2098  cc          INT $0x3
    bound1.go:18    0x2099  cc          INT $0x3
    bound1.go:18    0x209a  cc          INT $0x3
    bound1.go:18    0x209b  cc          INT $0x3
    bound1.go:18    0x209c  cc          INT $0x3
    bound1.go:18    0x209d  cc          INT $0x3
    bound1.go:18    0x209e  cc          INT $0x3
    bound1.go:18    0x209f  cc          INT $0x3
@rasky

This comment has been minimized.

Member

rasky commented Apr 2, 2016

/cc @brtzsnr -- i thought latest improvements should manage to remove this

@brtzsnr

This comment has been minimized.

Contributor

brtzsnr commented Apr 2, 2016

It doesn't transcend the zeroextension (MOVL CX, CX). Where does it show up?

@rasky

This comment has been minimized.

Member

rasky commented Apr 2, 2016

Sorry I don't understand the question. You mean which codebase is this from? I just spotted it while analyzing code generation to search for optimization opportunities related to bound checking.

@brtzsnr

This comment has been minimized.

Contributor

brtzsnr commented Apr 2, 2016

Thanks. It's good to keep track of which cases we miss.

@josharian josharian changed the title from Boundcheck not removed to cmd/compile: bounds check not removed Apr 3, 2016

@bradfitz bradfitz added the Performance label Apr 7, 2016

@bradfitz bradfitz added this to the Unplanned milestone Apr 7, 2016

@rasky rasky changed the title from cmd/compile: bounds check not removed to cmd/compile: bounds check not removed after "if index < length" Aug 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment