cmd/compile: large shifts miscompiled on riscv64

[bcmills]

#!watchflakes
post <- goarch == "riscv64" && pkg == "internal/chacha8rand" && test == "TestMarshal" && `Unmarshal: invalid ChaCha8 encoding`

https://build.golang.org/log/2031961837b8091c07edd2b489ea824efae86fe6:

--- FAIL: TestMarshal (0.00s)
    rand_test.go:42: #1: Unmarshal: invalid ChaCha8 encoding
FAIL
FAIL	internal/chacha8rand	0.006s

(attn @rsc @FiloSottile @golang/riscv64)

[bcmills]

Note that this is a test of a new API added for Go 1.22 (#61716).

[gopherbot]

Change https://go.dev/cl/543716 mentions this issue: internal/chacha8rand: disable ChaCha8 tests on certain platforms

[gopherbot]

Change https://go.dev/cl/543895 mentions this issue: Revert "math/rand/v2: add ChaCha8"

[4a6f656c]

/cc @markdryan

This code is triggering a code generation bug in the riscv64 rewrite rules (presumably introduced by https://go.dev/cl/528975 - 561bf04).

I'm investigating a fix.

[markdryan]

@4a6f656c Thanks for letting me know. I can reproduce this locally as well and can confirm that the issue goes away with https://go-review.googlesource.com/c/go/+/528975 and https://go-review.googlesource.com/c/go/+/535115 reverted. The reproducer is embarrassingly simple.

package main

import "fmt"

func badShift(c uint32) {
        fmt.Println(uint64(c) >> 56)
}

func badShiftSigned(c int32) {
        fmt.Println(int64(c) >> 56)
}

func main() {
        badShift(0xbadbaad)
        badShiftSigned(0xbadbaad)
}

This should print

0
0

but it actually prints

11
11

Without the patches, badShift generates

sll     t0,a0,0x20
srl     t0,t0,0x20
srl     a0,t0,0x38

and with them it generates

srlw    a0,a0,0x18 

which obviously isn't going to zero what was originally a 32 bit value in the same way that shifting by 0x38 will.

[markdryan]

The issue is also reproducible with variable shifts, e.g.,

func badShiftVar(c uint32, s uint64) {
	fmt.Println(uint64(c) >> s)
}

func badShiftSignedVar(c int32, s uint64) {
	fmt.Println(int64(c) >> s)
}

where s > 32

e.g.,

        badShiftVar(0xbadbaad, 56)
        badShiftSignedVar(0xbadbaad, 56)

also prints

11
11

[markdryan]

@4a6f656c do let me know if there's anything I can do to help.

Note that removing

// Avoid unnecessary zero and sign extension when right shifting.
(SRL <t> (MOVWUreg x) y) => (SRLW <t> x y)
(SRLI <t> [x] (MOVWUreg y)) => (SRLIW <t> [int64(x&31)] y)
(SRA <t> (MOVWreg x) y) => (SRAW <t> x y)
(SRAI <t> [x] (MOVWreg y)) => (SRAIW <t> [int64(x&31)] y)

and modifying the 32 bit shift rules directly, for example by replacing

(Rsh32Ux64 <t> x y) && !shiftIsBounded(v) => (AND (SRL <t> (ZeroExt32to64 x) y) (Neg32 <t> (SLTIU <t> [64] y)))
(Rsh32Ux(64|32|16|8) x y) && shiftIsBounded(v) => (SRL (ZeroExt32to64 x) y)

with

(Rsh32Ux64 <t> x y) && !shiftIsBounded(v) => (AND (SRLW <t> x                y) (Neg32 <t> (SLTIU <t> [32] y)))
(Rsh32Ux(64|32|16|8) x y) && shiftIsBounded(v) => (SRLW x                y)

does fix the issue, at the cost of missing out on the optimisation that is causing the problem, i.e., this disables the broken optimisation for uint64(c) >> s (where c is an uint32) rather than fixing it, while retaining the optimisations for shifts of 32 bit ints. Perhaps there's a more elegant solutions though, ...

[4a6f656c]

@markdryan thanks for investigating/confirming.

Unfortunately, my suggestion to optimise more generally has lead to a problem where by a valid type conversion and overflow shift is resulting in bad/incorrect code. The following two rules:

(SRL <t> (MOVWUreg x) y) => (SRLW <t> x y)
(SRLI <t> [x] (MOVWUreg y)) => (SRLIW <t> [int64(x&31)] y)

result in uint64(uint32(i)) >> 33 becoming SRLIW $1, y when it should obviously be zero (similarly with int64(int32(i)) >> 33). This can be resolved with appropriate bounds, however I think it is overall safer to take your original approach and move the SRAW/SRLW into the Rsh* rules - we just need a couple of (more carefully crafted) additional rules to keep the SRAIW/SRLIW optimisations.

[gopherbot]

Change https://go.dev/cl/545035 mentions this issue: cmd/compile: correct code generation for right shifts on riscv64

[markdryan]

Thanks @4a6f656c. I'll give the patch a good test this morning.

[gopherbot]

Change https://go.dev/cl/546020 mentions this issue: math/rand/v2: add ChaCha8