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

Jump to bottom

cmd/compile: unnecessary nil pointer check #40108

Open
randall77 opened this issue Jul 7, 2020 · 2 comments
Open

cmd/compile: unnecessary nil pointer check #40108

randall77 opened this issue Jul 7, 2020 · 2 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Unplanned

Comments

@randall77
Copy link
Contributor 

package main

type T [8]byte

func f(p *T, i int) byte {
	return p[i]
}

This code generates a nil pointer check on p.

	0x000e 00014 (/Users/khr/gowork/tmp1.go:6)	MOVQ	"".p+32(SP), DX
	0x0013 00019 (/Users/khr/gowork/tmp1.go:6)	TESTB	AL, (DX)           <- nil pointer check
	0x0015 00021 (/Users/khr/gowork/tmp1.go:6)	MOVQ	"".i+40(SP), AX
	0x001a 00026 (/Users/khr/gowork/tmp1.go:6)	NOP
	0x0020 00032 (/Users/khr/gowork/tmp1.go:6)	CMPQ	AX, $8
	0x0024 00036 (/Users/khr/gowork/tmp1.go:6)	JCC	56                       <- bounds check
	0x0026 00038 (/Users/khr/gowork/tmp1.go:6)	MOVBLZX	(DX)(AX*1), AX
	0x002a 00042 (/Users/khr/gowork/tmp1.go:6)	MOVB	AL, "".~r2+48(SP)
	0x002e 00046 (/Users/khr/gowork/tmp1.go:6)	MOVQ	16(SP), BP
	0x0033 00051 (/Users/khr/gowork/tmp1.go:6)	ADDQ	$24, SP
	0x0037 00055 (/Users/khr/gowork/tmp1.go:6)	RET

We don't really need a nil pointer check, as the index is bounded by the bounds check.
We can subsume the nil pointer check into the load.

This would require the nil pointer pass to know that indexes are bounded for indexed loads, so it can prove that the load still occurs in the zero page even with the largest possible index.
Maybe we need to keep some information from the prove pass around somehow?
@randall77 randall77 added this to the Unplanned milestone Jul 7, 2020
@cherrymui
Copy link
Member

If p is nil and i is out of bound, this may change the error produced from a nil pointer panic to a bound error. Not sure if this matters. If this does matter, we could do the nil check only on the out-of-bound code path, before issuing an out-of-bound panic.

@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 10, 2020
@mdempsky
Copy link
Member

The Go spec doesn't currently prescribe whether the dereference or bounds-check happens first. So as long as users aren't confused by the change in error message, this seems fine to me.

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@mdempsky @dmitshur @randall77 @gopherbot @cherrymui