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

Fix wrong arm64 scaled register format #61

Merged
merged 1 commit into from May 2, 2021
Merged

Fix wrong arm64 scaled register format #61

merged 1 commit into from May 2, 2021

Conversation

cuonglm
Copy link
Member

@cuonglm cuonglm commented Apr 20, 2021

Arm64 does not have scaled register format, casue snappy test failed for
current go tip:

	$ go version
	go version devel go1.17-24875e3880 Tue Apr 20 15:14:05 2021 +0000 darwin/arm64
	$ go test
	# github.com/golang/snappy
	./encode_arm64.s:385: arm64 doesn't support scaled register format
	./encode_arm64.s:675: arm64 doesn't support scaled register format
	asm: assembly of ./encode_arm64.s failed
	FAIL	github.com/golang/snappy [build failed]

See https://go-review.googlesource.com/c/go/+/289589

Arm64 does not have scaled register format, casue snappy test failed for
current go tip:

	$ go version
	go version devel go1.17-24875e3880 Tue Apr 20 15:14:05 2021 +0000 darwin/arm64
	$ go test
	# github.com/golang/snappy
	./encode_arm64.s:385: arm64 doesn't support scaled register format
	./encode_arm64.s:675: arm64 doesn't support scaled register format
	asm: assembly of ./encode_arm64.s failed
	FAIL	github.com/golang/snappy [build failed]

See https://go-review.googlesource.com/c/go/+/289589
@cuonglm
Copy link
Member Author

cuonglm commented Apr 22, 2021

Kindly ping @nigeltao

@nigeltao
Copy link
Contributor

@AWSjswinney can you have a look?

@AWSjswinney
Copy link
Contributor

As expected, the assembler in the 1.17 candidate emits the exact same instruction as previous versions (same for both changed lines):

b8af68c4        ldrsw   x4, [x6, x15]

Looks good to me.

@nigeltao nigeltao merged commit 33fc3d5 into golang:master May 2, 2021
@AWSjswinney
Copy link
Contributor

@nigeltao You might want to tag a release after this merge because this issue is going to become mainstream as soon as projects start building with Go 1.17, due in about a month.

@cuonglm
Copy link
Member Author

cuonglm commented Jun 29, 2021

@AWSjswinney @nigeltao it's not necessary anymore, since when Cherry decide to restore supporting for *1 syntax, see https://go-review.googlesource.com/c/go/+/328229

@AWSjswinney
Copy link
Contributor

Great! Thanks for pointing that out.

@nigeltao
Copy link
Contributor

nigeltao commented Jul 1, 2021

It might not be necessary, but it was easy to tag v0.0.4

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

Successfully merging this pull request may close these issues.

None yet

3 participants