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: uint16 arithmetic results different on arm between go 1.8 and go 1.7.5 #19270

Closed
nsd20463 opened this issue Feb 24, 2017 · 5 comments

Comments

Projects
None yet
7 participants
@nsd20463
Copy link
Contributor

commented Feb 24, 2017

On 32-bit arm linux I've found some code that produces an incorrect result when compiled with go 1.8.
Go 1.8 on amd64, and go 1.7.5 on 32-bit arm, produce the correct result.

This is illustrated by the little example at
https://play.golang.org/p/ji1Jc-OozF
which prints the byte-swapped result of its input.

The correct output is 0x888e. But the executable produced by
GOARCH=arm GOARM=7 go build test.go
prints 0xff8e.

@ALTree ALTree changed the title uint16 arithmetic results different on arm between go 1.8 and go 1.7.5 cmd/compile: uint16 arithmetic results different on arm between go 1.8 and go 1.7.5 Feb 24, 2017

@ianlancetaylor ianlancetaylor added this to the Go1.8.1 milestone Feb 24, 2017

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2017

(Lrot16 <t> x [c]) -> (OR (SLLconst <t> x [c&15]) (SRLconst <t> x [16-c&15]))

This rule was wrong. It should zero-extend x before right shift. On the other hand, it should use rotation instruction at first place. This is already fixed in Go 1.9 (Keith's CL on moving rotation generation to SSA). For fixing Go 1.8, I'll add a zero-extension there.

@cherrymui cherrymui self-assigned this Feb 24, 2017

@randall77

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2017

You should check all the other architectures to make sure we don't have the same bug there.

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2017

it should use rotation instruction at first place.

Correction: there is no sub-word rotation instruction. So lowering to left-shift OR right-shift (with zero-extension) is the right way.

You should check all the other architectures to make sure we don't have the same bug there.

Other architectures are correct. 386/AMD64 uses rotation instruction; ARM64 has a similar rule as ARM but does have zero-extension; Lrot8/16 are not generated on other architectures.

@gopherbot

This comment has been minimized.

Copy link

commented Mar 3, 2017

CL https://golang.org/cl/37718 mentions this issue.

gopherbot pushed a commit that referenced this issue Mar 3, 2017

[release-branch.go1.8] cmd/compile: add zero-extension before right s…
…hift when lowering Lrot on ARM

Fixes #19270.

Change-Id: Ie7538ff8465138a8bc02572e84cf5d00de7bbdd1
Reviewed-on: https://go-review.googlesource.com/37718
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>

@aclements aclements closed this Mar 21, 2017

@golang golang locked and limited conversation to collaborators Mar 21, 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.