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: miscompile of shift on GOOS=arm #64715
Comments
Also present in go-1.18,19,20. |
Weird. It seems to get through the SSA backend ok, but then there is a mis-assembly.
But if I disassemble the binary, there is
Indeed, if I add some unused code to
It disassembles to this:
|
Good lord, that's horrible: https://stackoverflow.com/questions/66487890/why-different-ranges-of-permitted-shift-values-for-arm-lsl-vs-lsr |
So we probably need to add a rule to rewrite right shift by 0 to left shitt by 0. |
@cuonglm That sounds right. Weirdly, our disassembler gets it right. Note the distinction between arg_imm5 and arg_imm5_32.
|
Shouldn't an ASHR/SRL of 0 just optimize away rather than changing one into another? But taking a look at the ARM manual, it looks like they've thought ahead about this edge case:
It looks like this might be something that should get handled at the assembler level to keep things consistent. This case also only applies to constant shifts, see the register version of the instruction vs the immediate version, so no extra work needs to be done on a register based shift of zero |
Certainly fixing the compiler to not issue 0 shifts is worth doing. But since we need to fix the assembler for this issue anyway, that compiler change is not needed to fix this bug. It would just be a performance improvement that could be done independently. (And this bug fix would be nice to get in for 1.22. Performance improvements would wait for 1.23.) |
Change https://go.dev/cl/549955 mentions this issue: |
This just got more complicated because there are not only shift instructions, but shifted register arguments (in fact, the former is implemented using MOV + the latter). We already give errors in some cases for register arguments shifted by bad amounts, like R0>>0. |
Change https://go.dev/cl/550335 mentions this issue: |
Right shift by 0 has bad semantics. Make sure if we try to right shift by 0, do a left shift by 0 instead. CL 549955 handled full instructions with this strange no-op encoding. This CL handles the shift done to instruction register inputs. (The former is implemented using the latter, but not until deep inside the assembler.) Update #64715 Change-Id: Ibfabb4b13e2595551e58b977162fe005aaaa0ad1 Reviewed-on: https://go-review.googlesource.com/c/go/+/550335 Run-TryBot: Cherry Mui <cherryyz@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Keith Randall <khr@google.com>
Right shifts, for some odd reasons, can encode shifts of constant 1-32 instead of 0-31. Left shifts, however, can encode shifts 0-31. When the shift amount is 0, arm recommends encoding right shifts using left shifts. Fixes golang#64715 Change-Id: Id3825349aa7195028037893dfe01fa0e405eaa51 Reviewed-on: https://go-review.googlesource.com/c/go/+/549955 Reviewed-by: Cherry Mui <cherryyz@google.com> Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Keith Randall <khr@google.com>
Right shift by 0 has bad semantics. Make sure if we try to right shift by 0, do a left shift by 0 instead. CL 549955 handled full instructions with this strange no-op encoding. This CL handles the shift done to instruction register inputs. (The former is implemented using the latter, but not until deep inside the assembler.) Update golang#64715 Change-Id: Ibfabb4b13e2595551e58b977162fe005aaaa0ad1 Reviewed-on: https://go-review.googlesource.com/c/go/+/550335 Run-TryBot: Cherry Mui <cherryyz@google.com> TryBot-Result: Gopher Robot <gobot@golang.org> Reviewed-by: Cherry Mui <cherryyz@google.com> LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Keith Randall <khr@google.com>
Go version
go version devel go1.22-f2d243db8f Wed Dec 13 16:20:09 2023 +0000 linux/amd64
What operating system and processor architecture are you using (
go env
)?What did you do?
NB: filing this issue on behalf of Jan Mercl, who discovered and reported the problem here
Run this program on a GOOS=linux GOARCH=arm machine:
What did you expect to see?
Expected:
What did you see instead?
Got instead:
Also worth noting that building with
-gcflags="-l -N"
makes the problem go away.The text was updated successfully, but these errors were encountered: