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.

Sign up for GitHub

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/internal/obj/arm64: remove the transition from $0 to ZR [1.18~1.19 backport] #57477

Closed
felix021 opened this issue Dec 27, 2022 · 4 comments
Closed

cmd/internal/obj/arm64: remove the transition from $0 to ZR [1.18~1.19 backport] #57477

felix021 opened this issue Dec 27, 2022 · 4 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@felix021
Copy link

felix021 commented Dec 27, 2022 •
edited

Found fix in this commit 9f0f87c to master & go1.20rc1. Is it planned to be backported to previous versions?

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

$ go version
go version go1.19.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes (for 1.19.4; 1.20 is not released for now)

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

Mac OS, x86_64 (working on arm64 libraries)

What did you do?

Working on a library to support arm64 processors, the compiler generated binary with unexpected byte code.

Below is a minimal POC:

TEXT ·test(SB), $0-0
    WORD $11223344
    WORD $55667788
    WORD $00000000
    WORD $00000000
    RET

cross compile to generate arm64 binary:

$ GOARCH=arm64 go build

objdump to see the byte code generated:

GOARCH=arm64 go tool objdump test

What did you expect to see?

Same bytecode as specified in the asm source.

What did you see instead?

TEXT main.test.abi0(SB) /Users/xxx/arm64/test.s
  test.s:2              0x1000549d0             00ab4130                ?
  test.s:3              0x1000549d4             03516c4c                ?
  test.s:4              0x1000549d8             03516c4c                ?
  test.s:5              0x1000549dc             03516c4c                ?
  test.s:6              0x1000549e0             d65f03c0                RET

3rd & 4th instructions are the SAME as the 2nd one, not 0s.
@seankhliao seankhliao changed the title cmd/internal: remove the transition from $0 to ZR [1.15~1.19 backport] cmd/internal/obj/arm64: remove the transition from $0 to ZR [1.18~1.19 backport] Dec 27, 2022
@seankhliao
Copy link
Member

cc @golang/arm ?

@seankhliao seankhliao added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Dec 27, 2022
@cherrymui
Copy link
Member

I don't think we want to backport this. This is not a regression. It had been like that for a long time until the fix. Also, that CL introduced issues that requires further fixes. If we backport that CL, we'd need to backport more CLs.

the compiler generated binary with unexpected byte code

Does the compiler generate wrong code, instead of hand-written assembly with WORD directive?

Thanks.

@cherrymui cherrymui added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Dec 27, 2022
@felix021
Copy link
Author

felix021 commented Dec 27, 2022 •
edited

Thanks for your explanation.

The WORD directive is generated by another tool for better performance.

@cherrymui
Copy link
Member

Thanks. I don't think we will backport the CL, which is a relatively invasive change. If it matters, it might be reasonable to consider doing a very targeted fix for WORD $0 (not $0 to ZR in general). But given that it had been like that for a long time, I'm not sure. Closing for now. Thanks.

@cherrymui cherrymui closed this as not planned Won't fix, can't repro, duplicate, stale Dec 27, 2022
@golang golang locked and limited conversation to collaborators Dec 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@felix021 @gopherbot @seankhliao @cherrymui