cmd/compile: miscompilation of zero extension on ppc64le

[ianlancetaylor]

This file is miscompiled on ppc64le:

package main

//go:noinline
func g(i uint64) uint64 {
        return uint64(uint32(i))
}

var sink uint64

func main() {
        for i := uint64(0); i < 1; i++ {
                i32 := int32(i - 1)
                sink = uint64((uint32(i32) << 1) ^ uint32((i32 >> 31)))
                x := g(uint64(i32))
                if x != uint64(uint32(i32)) {
                        panic(x)
                }
        }
}

On amd64 with tip, or on ppc64le with Go 1.11.5, this program runs without error. On ppc64le GNU/Linux with tip, the program panics:

panic: 4294967295

goroutine 1 [running]:
main.main()
    /home/ian/foo.go:16 +0xa8
exit status 2

Using objdump to look at the executable, I see this:

/home/ian/foo.go:12
   621cc:       ff ff 83 38     addi    r4,r3,-1
   621d0:       28 00 85 78     rldic   r5,r4,0,32
   621d4:       30 00 a1 90     stw     r5,48(r1)
/home/ian/foo.go:13
   621d8:       3c 08 a6 54     rlwinm  r6,r5,1,0,30
   621dc:       70 fe a7 7c     srawi   r7,r5,31
   621e0:       78 3a c6 7c     xor     r6,r6,r7
   621e4:       28 00 c6 78     rldic   r6,r6,0,32
   621e8:       10 00 e0 3f     lis     r31,16
   621ec:       18 3d df f8     std     r6,15640(r31)
/home/ian/foo.go:14
   621f0:       b4 07 84 7c     extsw   r4,r4
   621f4:       20 00 81 f8     std     r4,32(r1)
   621f8:       89 ff ff 4b     bl      62180 <main.g>
   621fc:       28 00 61 e8     ld      r3,40(r1)
/home/ian/foo.go:15
   62200:       32 00 81 e8     lwa     r4,48(r1)
   62204:       00 20 23 7c     cmpd    r3,r4
   62208:       b0 ff 82 41     beq     621b8 <main.main+0x28>
   6220c:       14 00 00 48     b       62220 <main.main+0x90>
   62210:       00 00 e1 eb     ld      r31,0(r1)
   62214:       a6 03 e8 7f     mtlr    r31
   62218:       40 00 21 38     addi    r1,r1,64
   6221c:       20 00 80 4e     blr

The variable i32 is stored at 48(r1). At PC 62200, the value is loaded, but it is loaded with an lwa instruction, which sign extends the value. This corresponds to the Go expression uint64(uint32(i32)), which should zero extend the value, presumably using lwz.

This is a miscompilation of valid code so it is a release blocker for 1.12.

CC @randall77 @laboger

[ianlancetaylor]

According to a git bisect, the error was introduced at https://golang.org/cl/129875 (8dbd9afbb05c77ca8426256d172f9e05fe48a0f0).

[ianlancetaylor]

CC @martisch

[ianlancetaylor]

CC @dr2chase

[ianlancetaylor]

CC @rasky

[cherrymui]

The comment at https://go-review.googlesource.com/c/go/+/129875/8/src/cmd/compile/internal/ssa/gen/PPC64.rules#903 says

// Note that MOV??reg returns a 64-bit int, x is not necessarily that wide
// This may interact with other patterns in the future. (Compare with arm64)

I think this is exactly the reason of this failure (yah, the "future" has come).

[cherrymui]

Before lower,

v10 (12) = Trunc64to32 <int32> v9 (i32[int32])
...
v29 (+15) = ZeroExt32to64 <uint64> v10
v30 (15) = Neq64 <bool> v27 v29
If v30 → b6 b4 (15)

Note that v29 has a 64-bit type.

After lower,

v10 (12) = MOVWZreg <int32> v9 (i32[int32])
...
v28 (+15) = CMP <flags> v27 v10
NE v28 → b6 b4 (15)

Note that v10, MOVWZ is a 32-bit to 64-bit zero extension, but has type int32. And the rule folds v10 to v29, which folds into the CMP, v28.

After regalloc,

v10 (12) = MOVWZreg <int32> v9 : R5 (i32[int32])
v42 (12) = StoreReg <int32> v10 : i32[int32]
...
v30 (15) = LoadReg <int32> v42 : R4
v28 (+15) = CMP <flags> v27 v30
NE v28 → b6 b4 (unlikely) (15)

v10 is spilled (because of the call to g), but the spill and reload are just 32-bit load/store.

[cherrymui]

On all architectures but PPC64, Trunc64to32 is a no-op. Why it is not on PPC64?

AMD64.rules:(Trunc64to32 x) -> x
ARM64.rules:(Trunc64to32 x) -> x
MIPS64.rules:(Trunc64to32 x) -> x
PPC64.rules:(Trunc64to32 x) && isSigned(x.Type) -> (MOVWreg x)
PPC64.rules:(Trunc64to32 x) -> (MOVWZreg x)
S390X.rules:(Trunc64to32 x) -> x
Wasm.rules:(Trunc64to(32|16|8) x) -> x

[laboger]

That was an old comment and I don't know where it originated. The rules beneath the comment are not mixing zero truncates with signed loads as this case does. I think more likely it has to do with the Trunc rules immediately above that comment, I think there was a concern about the change I made there.

On Fri, Jan 25, 2019 at 4:09 PM cherrymui notifications@github.com wrote:

The comment at https://go-review.googlesource.com/c/go/+/129875/8/src/cmd/compile/internal/ssa/gen/PPC64.rules#903 says

// Note that MOV??reg returns a 64-bit int, x is not necessarily that wide // This may interact with other patterns in the future. (Compare with arm64)

I think this is exactly the reason of this failure (yah, the "future" has come).

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/golang/go/issues/29943#issuecomment-457748135, or mute the thread https://github.com/notifications/unsubscribe-auth/AI_wjDe9X4LqVj3LBCsiORxNViN3cWw3ks5vG4CYgaJpZM4aTsXs .

[ianlancetaylor]

I don't understand these rules (but I'm not familiar with this code). Truncating a 64-bit value to a 32-bit value is an operation that is independent of whether the type of the operand is signed.

[cherrymui]

It is not the rule immediately followed the comment. It is this

(MOVWZreg y:(MOVWZreg _)) -> y // repeat

The outer MOVWZ is ZeroExt32to64, the inner one is Trunc64to32. The reason is same: y's type is not wide enough.

[cherrymui]

On ARM64, MIPS64, and S390X, it is (MOVWUreg x:(MOVWUreg _)) -> (MOVDreg x), to ensure that we don't narrow the type of the outer op.

[ianlancetaylor]

What is our path forward here so that we can unblock the 1.12 release? Should we roll back this CL, or part of it?

[gopherbot]

Change https://golang.org/cl/159760 mentions this issue: cmd/compile: base PPC64 trunc rules on final type, not op type