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.

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: spurious nil check in len(slice) #7316

Closed
rsc opened this issue Feb 12, 2014 · 8 comments
Closed

cmd/compile: spurious nil check in len(slice) #7316

rsc opened this issue Feb 12, 2014 · 8 comments

Comments

@rsc
Copy link
Contributor

rsc commented Feb 12, 2014

package main

func f() int {
buf := make([]byte, 1024)
return len(buf)
}

go tool 6g -S x.go

    000000 00000 (x.go:3)   TEXT    "".f+0(SB),$1056-8
    000000 00000 (x.go:3)   MOVQ    2208(GS),CX
    0x0009 00009 (x.go:3)   LEAQ    -928(SP),AX
    0x0011 00017 (x.go:3)   CMPQ    AX,(CX)
    0x0014 00020 (x.go:3)   JHI ,29
    0x0016 00022 (x.go:3)   CALL    ,runtime.morestack8(SB)
    0x001b 00027 (x.go:3)   JMP ,0
    0x001d 00029 (x.go:3)   SUBQ    $1056,SP
    0x0024 00036 (x.go:3)   FUNCDATA    $2,"".gcargs·0+0(SB)
    0x0024 00036 (x.go:3)   FUNCDATA    $3,"".gclocals·1+0(SB)
    0x0024 00036 (x.go:4)   LEAQ    "".autotmp_0000+0(SP),DI
    0x0028 00040 (x.go:4)   MOVQ    $0,AX
    0x002b 00043 (x.go:4)   MOVQ    $128,CX
    0x0032 00050 (x.go:4)   REP ,
    0x0033 00051 (x.go:4)   STOSQ   ,
    0x0035 00053 (x.go:4)   LEAQ    "".autotmp_0000+0(SP),BX
    0x0039 00057 (x.go:4)   MOVQ    BX,"".autotmp_0001+1024(SP)
    0x0041 00065 (x.go:4)   MOVQ    $1024,"".buf+1040(SP)
    0x004d 00077 (x.go:4)   MOVQ    $1024,"".buf+1048(SP)
    0x0059 00089 (x.go:4)   MOVQ    "".autotmp_0001+1024(SP),BX
    0x0061 00097 (x.go:4)   MOVQ    BX,"".buf+1032(SP)
    0x0069 00105 (x.go:4)   CMPQ    "".buf+1032(SP),$0
    0x0072 00114 (x.go:4)   JEQ $1,140
    0x0074 00116 (x.go:5)   MOVQ    "".buf+1040(SP),BX
    0x007c 00124 (x.go:5)   MOVQ    BX,"".~anon0+1064(FP)
    0x0084 00132 (x.go:5)   ADDQ    $1056,SP
    0x008b 00139 (x.go:5)   RET ,
    0x008c 00140 (x.go:4)   MOVL    AX,(NONE)
    0x0093 00147 (x.go:5)   JMP ,116

the CMPQ at 0x0069 is a nil check during len(buf), which is bogus.

also, why is the initialization of the header not registerizing better and then being
optimized away?
i would expect this code to be something like

SUBQ $1056, SP
LEAQ autotmp+0(SP), DI
MOVQ $0, AX
MOVQ $128, CX
REP
STOSQ
MOVQ $1024, result+1064(FP)
RET
@rsc
Copy link
Contributor Author

rsc commented Feb 12, 2014

Comment 1:

Should understand these inefficiencies for Go 1.3.

Labels changed: added release-go1.3.

Owner changed to @rsc.

Status changed to Accepted.

@gopherbot
Copy link

Comment 2:

CL https://golang.org/cl/96720043 mentions this issue.

@josharian
Copy link
Contributor

Comment 3:

I've started on the registerization question as well (with lots of help from Daniel
Morsing).

Status changed to Started.

@gopherbot
Copy link

Comment 4:

CL https://golang.org/cl/91850043 mentions this issue.

@rsc
Copy link
Contributor Author

rsc commented May 11, 2014

Comment 5:

Labels changed: added release-go1.4, removed release-go1.3.

@josharian
Copy link
Contributor

Comment 6:

This issue was updated by revision 03c0f3f.

LGTM=rsc
R=rsc, daniel.morsing, bradfitz
CC=dave, golang-codereviews
https://golang.org/cl/91850043
Committer: Russ Cox 

@josharian
Copy link
Contributor

Comment 7:

Release-None per rsc's comment in CL 96720043.

Labels changed: added release-none, repo-main, removed release-go1.4.

@rsc rsc self-assigned this Sep 2, 2014
@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title cmd/gc: spurious nil check in len(slice) cmd/compile: spurious nil check in len(slice) Jun 8, 2015
@bradfitz bradfitz removed the Started label Jan 6, 2017
@ALTree
Copy link
Member

ALTree commented Feb 2, 2017

go1.8 gives (morestack excluded):

"".f t=1 size=107 args=0x8 locals=0x408
	0x0017 00023 SUBQ	$1032, SP
	0x001e 00030 MOVQ	BP, 1024(SP)
	0x0026 00038 LEAQ	1024(SP), BP
	0x002e 00046 FUNCDATA	$0, gclocals·2a5305abe05176240e61b8620e19a815(SB)
	0x002e 00046 FUNCDATA	$1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
	0x002e 00046 LEAQ	""..autotmp_1(SP), DI
	0x0032 00050 XORPS	X0, X0
	0x0035 00053 DUFFZERO	$0
	0x0048 00072 MOVQ	$1024, "".~r0+1040(FP)
	0x0054 00084 MOVQ	1024(SP), BP
	0x005c 00092 ADDQ	$1032, SP
	0x0063 00099 RET

The spurious nilcheck mentioned in the title is no longer there, but maybe @josharian can decide if the registerization is now good enough (and we can close this) or it needs to stay open.

@golang golang locked and limited conversation to collaborators Feb 2, 2018
@rsc rsc removed their assignment Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants