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: optimize array indexing for small unsigned int types #35491

Closed
rillig opened this issue Nov 10, 2019 · 1 comment
Closed

cmd/compile: optimize array indexing for small unsigned int types #35491

rillig opened this issue Nov 10, 2019 · 1 comment

Comments

@rillig
Copy link
Contributor

@rillig rillig commented Nov 10, 2019

What version of Go are you using (go version)?

go version go1.13.1 windows/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

What did you do?

func digit8(i uint8) byte {
	if i < 10 {
		return "0123456789"[i]
	}
	return '?'
}

func digit16(i uint16) byte {
	if i < 10 {
		return "0123456789"[i]
	}
	return '?'
}

func digit32(i uint32) byte {
	if i < 10 {
		return "0123456789"[i]
	}
	return '?'
}

func digit64(i uint64) byte {
	if i < 10 {
		return "0123456789"[i]
	}
	return '?'
}

What did you expect to see?

The generated code for all these variants is pretty much the same.
None of them calls runtime.panicIndex.

What did you see instead?

The variants for uint8, uint16 and uint32 add unnecessary code for index checking.

For example:

TEXT ipaddr.digit16(SB)
  bytestr.go:136                 subq     $0x18, sp
                                 movq     bp, 0x10(sp)
                                 leaq     0x10(sp), bp
  bytestr.go:137                 movzx    0x20(sp), ax
                                 cmpw     $0xa, ax
                                 jae      0x19b0 (line 470, bytestr.go:138)
  bytestr.go:138                 cmpq     $0xa, ax
                                 jae      0x19bf (line 475, bytestr.go:140)
                                 leaq     0(ip), cx             [3:7]R_PCREL:go.string."0123456789"
                                 movzx    0(cx)(ax*1), ax
                                 movb     al, 0x28(sp)
                                 movq     0x10(sp), bp
                                 addq     $0x18, sp
                                 ret

  bytestr.go:140        0x19b0   movb     $0x3f, 0x28(sp)
                                 movq     0x10(sp), bp
                                 addq     $0x18, sp
                                 ret

  bytestr.go:138        0x19bf   movl     $0xa, cx
                                 call     0x19c9                [1:5]R_CALL:runtime.panicIndex
                        0x19c9   nopl

This is not restricted to amd64. On arm and 386, the comparison is executed redundantly:

  bytestr.go:137                 movhu    0x10(r13), R2
                                 movw     R2, R3
                                 cmp      $10, R3
                                 b        .cs 0x1ac0
  bytestr.go:138                 movw     R2, R0
                                 cmp      $10, R0
                                 b        .cs 0x1acc

  bytestr.go:137                 movzx    0xc(sp), dx
                                 cmpw     $0xa, dx
                                 jae      0x1a32 (line 528, bytestr.go:138)
  bytestr.go:138                 movzx    dx, ax
                                 cmpl     $0xa, ax
                                 jae      0x1a3b (line 532, bytestr.go:140)
@ALTree

This comment has been minimized.

Copy link
Member

@ALTree ALTree commented Nov 10, 2019

Thanks for the report.

This has already been fixed on tip by cl 174704. For example:

$ gotip version
go version devel +a0262b201f Sat Nov 9 05:51:04 2019 +0000 linux/amd64

$ cat test.go
package p

func digit16(i uint16) byte {
	if i < 10 {
		return "0123456789"[i]
	}
	return '?'
}
$ gotip tool compile -S test.go | grep -A20 digit16

"".digit16 STEXT nosplit size=33 args=0x10 locals=0x0
	0x0000 00000 (test.go:3)	TEXT	"".digit16(SB), NOSPLIT|ABIInternal, $0-16
	0x0000 00000 (test.go:3)	PCDATA	$0, $-2
	0x0000 00000 (test.go:3)	PCDATA	$1, $-2
	0x0000 00000 (test.go:3)	FUNCDATA	$0, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
	0x0000 00000 (test.go:3)	FUNCDATA	$1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
	0x0000 00000 (test.go:3)	FUNCDATA	$2, gclocals·568470801006e5c0dc3947ea998fe279(SB)
	0x0000 00000 (test.go:4)	PCDATA	$0, $0
	0x0000 00000 (test.go:4)	PCDATA	$1, $0
	0x0000 00000 (test.go:4)	MOVWLZX	"".i+8(SP), AX
	0x0005 00005 (test.go:4)	CMPW	AX, $10
	0x0009 00009 (test.go:4)	JCC	27
	0x000b 00011 (test.go:5)	PCDATA	$0, $1
	0x000b 00011 (test.go:5)	LEAQ	go.string."0123456789"(SB), CX
	0x0012 00018 (test.go:5)	PCDATA	$0, $0
	0x0012 00018 (test.go:5)	MOVBLZX	(CX)(AX*1), AX
	0x0016 00022 (test.go:5)	MOVB	AL, "".~r1+16(SP)
	0x001a 00026 (test.go:5)	RET
	0x001b 00027 (test.go:7)	MOVB	$63, "".~r1+16(SP)
	0x0020 00032 (test.go:7)	RET
	0x0000 0f b7 44 24 08 66 83 f8 0a 73 10 48 8d 0d 00 00  ..D$.f...s.H....
	0x0010 00 00 0f b6 04 01 88 44 24 10 c3 c6 44 24 10 3f  .......D$...D$.?

The change will be included in Go1.14, so I'm closing this issue, since I don't think there is anything else to do here. Feel free to comment if you disagree.

@ALTree ALTree closed this Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.