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: Duplicated sequence of instructions in runtime.adjustframe on AMD64 #49167

Closed
jake-ciolek opened this issue Oct 26, 2021 · 3 comments
Closed

Comments

@jake-ciolek
Copy link
Contributor

@jake-ciolek jake-ciolek commented Oct 26, 2021

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

go tip

Does this issue reproduce with the latest release?

Yes

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

go env Output
Darwin AMD64

What did you do?

Looked at the assembly code of runtime.adjustframe.

What did you expect to see?

movl	$1, %eax
movq	128(%rsp), %rbp
addq	$136, %rsp
retq

I expected to see this once since there's a ret.

What did you see instead?

movl	$1, %eax
movq	128(%rsp), %rbp
addq	$136, %rsp
retq
movl	$1, %eax
movq	128(%rsp), %rbp
addq	$136, %rsp
retq
movl	$1, %eax
movq	128(%rsp), %rbp
addq	$136, %rsp
retq

Perhaps I am missing something and there's a valid reason for this, but it seems to me that we could use this sequence once and save 40+ bytes of instructions. Not sure if it's occuring anywhere else.

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 26, 2021

If you look at the line table information, you'll see that there are different line numbers associated with each of those instruction blocks. There are three of them because there are three different return true statements in the function. It's not possible for a Go compiler to merge instruction sequences in the general case, because that will cause a panic to show the wrong line number. This particular instruction sequence probably can't panic, but a signal is still a possibility. It's not clear to me whether these instructions can be merged or not.

@randall77
Copy link
Contributor

@randall77 randall77 commented Oct 26, 2021

See #24936 (#24936 (comment))
I had a CL to do something like this, never really got around to completing it https://go-review.googlesource.com/c/go/+/265100

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 26, 2021

Thanks, closing as dup of #24936.

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

No branches or pull requests

3 participants