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

Jump to bottom

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
Closed

cmd/compile: uint16 arithmetic results different on arm between go 1.8 and go 1.7.5 #19270

nsd20463 opened this issue Feb 24, 2017 · 5 comments
Labels
FrozenDueToAge
Milestone
Go1.8.1

Comments

@nsd20463
Copy link
Contributor

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
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Feb 24, 2017
@ianlancetaylor ianlancetaylor added this to the Go1.8.1 milestone Feb 24, 2017
@ianlancetaylor
Copy link
Contributor

CC @randall77 @josharian

@cherrymui
Copy link
Member 

(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
Copy link
Contributor

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

@cherrymui
Copy link
Member

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
Copy link

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

gopherbot pushed a commit that referenced this issue Mar 3, 2017
@cherrymui
[release-branch.go1.8] cmd/compile: add zero-extension before right s…
b43fabf 
…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 removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label 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.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.8.1
Development

No branches or pull requests
7 participants
@aclements @ianlancetaylor @ALTree @nsd20463 @randall77 @gopherbot @cherrymui