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: prefer to cheaply rematerialize than copy registers #24132

Closed
josharian opened this issue Feb 26, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@josharian
Copy link
Contributor

commented Feb 26, 2018

package p

func f(x int) (uint32, uint32) {
	var a, b uint32
	for {
		a++
		if x == 0 {
			break
		}
		x--
		b += 2
	}
	return a, b
}

This compiles as:

"".f STEXT nosplit size=33 args=0x10 locals=0x0
	0x0000 00000 (x.go:3)	TEXT	"".f(SB), NOSPLIT, $0-16
	0x0000 00000 (x.go:3)	FUNCDATA	$0, gclocals·f207267fbf96a0178e8758c6e3e0ce28(SB)
	0x0000 00000 (x.go:3)	FUNCDATA	$1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
	0x0000 00000 (x.go:3)	MOVQ	"".x+8(SP), AX
	0x0005 00005 (x.go:3)	XORL	CX, CX
	0x0007 00007 (x.go:3)	MOVL	CX, DX
	0x0009 00009 (x.go:5)	JMP	17
	0x000b 00011 (x.go:10)	DECQ	AX
	0x000e 00014 (x.go:11)	ADDL	$2, DX
	0x0011 00017 (x.go:6)	INCL	CX
	0x0013 00019 (x.go:7)	TESTQ	AX, AX
	0x0016 00022 (x.go:7)	JNE	11
	0x0018 00024 (x.go:13)	MOVL	CX, "".~r1+16(SP)
	0x001c 00028 (x.go:13)	MOVL	DX, "".~r2+20(SP)
	0x0020 00032 (x.go:13)	RET

This issue is about instruction 0x0007, MOVL CX, DX. I think we should prefer XORL DX, DX. The reasoning is that it is shorter (2 bytes instead of 4) and avoids false dependencies between registers. This is only preferable when rematerialization is cheaper than a register copy, which are special but common cases, like zeroing.

I believe that the modification to regalloc should occur in processDest, but that's as far as I got.

cc @cherrymui @randall77

@josharian josharian added this to the Unplanned milestone Feb 26, 2018

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

I thought we don't emit copy for rematerializeable values. Maybe there are some cases we missed, specifically in this case, the move to satisfy a Phi?

@randall77

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2018

Both instructions are 2 bytes, are they not?

You're right, it is in processDest. I think you might just need to reverse the order in which c==nil and the rematerializeable flag are checked.

@gopherbot

This comment has been minimized.

Copy link

commented Mar 16, 2018

Change https://golang.org/cl/101076 mentions this issue: cmd/compile: prefer rematerialization to copying

@gopherbot gopherbot closed this in dafca7d Mar 28, 2018

@golang golang locked and limited conversation to collaborators Mar 28, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.