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: convert some const shifts into load offsets #19286

Closed
josharian opened this issue Feb 25, 2017 · 7 comments

Comments

Projects
None yet
6 participants
@josharian
Copy link
Contributor

commented Feb 25, 2017

package p

func top(x uint64) uint32 {
	return uint32(x >> 32)
}

Currently compiles to:

MOVQ	"".x+8(FP), AX
SHRQ	$32, AX
MOVL	AX, "".~r1+16(FP)
RET

But instead of shifting, the load offset could be adjusted:

MOVL	"".x_hi+12(FP), AX
MOVL	AX, "".~r1+16(FP)
RET

Probably could even be done in the generic rules.

cc @randall77 @mundaym @martisch @TocarIP

@josharian josharian added this to the Go1.9 milestone Feb 25, 2017

@josharian josharian changed the title cmd/compile: merge some const shifts into load offsets cmd/compile: convert some const shifts into load offsets Feb 25, 2017

@mundaym

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

Can all the chips we care about do store-to-load forwarding with mismatched sizes? Might be better to keep this out of the generic rules unless we are certain that they all can.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 25, 2017

This come up often?

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2017

Can all the chips we care about do store-to-load forwarding with mismatched sizes? Might be better to keep this out of the generic rules unless we are certain that they all can.

Good point. I'm not even sure that amd64 handles this well. Moving to unplanned for now.

This come up often?

Haven't investigated.

@josharian josharian modified the milestones: Unplanned, Go1.9 Feb 25, 2017

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2017

Also, each additional rule incurs a cost - and it's not clear the cost here (extra check for this case at compile-time) will outweigh the benefit.

@TocarIP

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2017

I've prototyped this as a target-specific rule, it triggers once during compilation, and provides no performance benefit for simple benchmark (calling top in a loop). I think this is not worth implementing.

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Mar 10, 2017

Thanks for running it down, @TocarIP. Closing.

@josharian josharian closed this Mar 10, 2017

@josharian

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2017

Not proposing to reopen this, but FWIW, this does occur in the hashmap implementation. Pulling the repeated tophash calculation into its own function for demo purposes:

func tophash(hash uintptr) uint8 {
	top := uint8(hash >> (sys.PtrSize*8 - 8))
	if top < minTopHash {
		top += minTopHash
	}
	return top
}

compiles to:

"".tophash STEXT nosplit size=21 args=0x10 locals=0x0
	0x0000 00000 (/Users/josh/go/src/runtime/hashmap.go:1044)	TEXT	"".tophash(SB), NOSPLIT, $0-16
	0x0000 00000 (/Users/josh/go/src/runtime/hashmap.go:1044)	FUNCDATA	$0, gclocals·f207267fbf96a0178e8758c6e3e0ce28(SB)
	0x0000 00000 (/Users/josh/go/src/runtime/hashmap.go:1044)	FUNCDATA	$1, gclocals·33cdeccccebe80329f1fdbee7f5874cb(SB)
	0x0000 00000 (/Users/josh/go/src/runtime/hashmap.go:1044)	MOVQ	"".hash+8(SP), AX
	0x0005 00005 (/Users/josh/go/src/runtime/hashmap.go:1045)	SHRQ	$56, AX
	0x0009 00009 (/Users/josh/go/src/runtime/hashmap.go:1046)	CMPB	AL, $4
	0x000b 00011 (/Users/josh/go/src/runtime/hashmap.go:1046)	JCC	16
	0x000d 00013 (/Users/josh/go/src/runtime/hashmap.go:1047)	ADDL	$4, AX
	0x0010 00016 (/Users/josh/go/src/runtime/hashmap.go:1049)	MOVB	AL, "".~r1+16(SP)
	0x0014 00020 (/Users/josh/go/src/runtime/hashmap.go:1049)	RET

The MOVQ/SHRQ at the beginning could just be a MOVB.

@golang golang locked and limited conversation to collaborators Jun 7, 2018

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.