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: struct equality code optimization #39428

Open
randall77 opened this issue Jun 5, 2020 · 2 comments
Open

cmd/compile: struct equality code optimization #39428

randall77 opened this issue Jun 5, 2020 · 2 comments

Comments

@randall77
Copy link
Contributor

@randall77 randall77 commented Jun 5, 2020

type S struct {
	a, b, c string
}
type A [3]string

func f(x, y S) bool {
	return x == y
}
func g(x, y A) bool {
	return x == y
}

The array equality function looks optimal. It compares each length in turn and branches to "return false" if any don't match. If all the lengths match, then it calls memequal in a loop for each string.

The code for struct equality looks worse.

	0x0021 00033 (<autogenerated>:1)	MOVQ	"".p+48(SP), AX
	0x0026 00038 (<autogenerated>:1)	MOVQ	8(AX), CX
	0x002a 00042 (<autogenerated>:1)	MOVQ	"".q+56(SP), DX
	0x002f 00047 (<autogenerated>:1)	MOVQ	(DX), BX
	0x0032 00050 (<autogenerated>:1)	MOVQ	(AX), SI
	0x0035 00053 (<autogenerated>:1)	CMPQ	8(DX), CX
	0x0039 00057 (<autogenerated>:1)	JEQ	198
	0x003f 00063 (<autogenerated>:1)	XORL	CX, CX
	0x0041 00065 (<autogenerated>:1)	TESTB	CL, CL
	0x0043 00067 (<autogenerated>:1)	JEQ	194

	0x00c6 00198 (<autogenerated>:1)	MOVQ	SI, (SP)
	0x00ca 00202 (<autogenerated>:1)	MOVQ	BX, 8(SP)
	0x00cf 00207 (<autogenerated>:1)	MOVQ	CX, 16(SP)
	0x00d4 00212 (<autogenerated>:1)	CALL	runtime.memequal(SB)
	0x00d9 00217 (<autogenerated>:1)	MOVBLZX	24(SP), CX
	0x00de 00222 (<autogenerated>:1)	MOVQ	"".p+48(SP), AX
	0x00e3 00227 (<autogenerated>:1)	MOVQ	"".q+56(SP), DX
	0x00e8 00232 (<autogenerated>:1)	JMP	65

The length-same (calling memequal) and length-different branches recombine, requiring retesting of the result, which then skips over the remaining tests (and not all the rest, but one at a time).

@josharian

@randall77 randall77 added this to the Go1.16 milestone Jun 5, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Jun 5, 2020

@randall77 Thanks for opening this. This issue was missing one of the Needs{Investigation,Decision,Fix} labels that triaged issues should have. I've added NeedsInvestigation, but you can pick another if an issue you're reporting is in a different state. Thanks.

@josharian
Copy link
Contributor

@josharian josharian commented Jun 6, 2020

Thanks. I’m still AFK but I suspect (purely from the issue text) that it’d be easier/better to fix this by strengthening the shortcircuit pass more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.