cmd/compile: miscompilation of zero extension on ppc64le #29943

ianlancetaylor · 2019-01-25T21:33:52Z

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

The text was updated successfully, but these errors were encountered:

ianlancetaylor · 2019-01-25T21:59:17Z

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

ianlancetaylor · 2019-01-25T22:00:02Z

CC @martisch

ianlancetaylor · 2019-01-25T22:00:11Z

CC @dr2chase

ianlancetaylor · 2019-01-25T22:00:40Z

CC @rasky

cherrymui · 2019-01-25T22:08:39Z

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 · 2019-01-25T22:17:45Z

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 · 2019-01-25T22:24:30Z

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 · 2019-01-25T22:30:09Z

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 ***@***.***> 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 <#29943 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AI_wjDe9X4LqVj3LBCsiORxNViN3cWw3ks5vG4CYgaJpZM4aTsXs> .

ianlancetaylor · 2019-01-25T22:36:50Z

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 · 2019-01-25T22:36:55Z

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 · 2019-01-25T22:40:02Z

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 · 2019-01-25T23:11:53Z

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 · 2019-01-26T19:57:59Z

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

ianlancetaylor added NeedsFix release-blocker labels Jan 25, 2019

ianlancetaylor added this to the Go1.12 milestone Jan 25, 2019

gopherbot closed this as completed in d585f04 Jan 27, 2019

golang locked and limited conversation to collaborators Jan 27, 2020

gopherbot added the FrozenDueToAge label Jan 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/compile: miscompilation of zero extension on ppc64le #29943

cmd/compile: miscompilation of zero extension on ppc64le #29943

ianlancetaylor commented Jan 25, 2019

ianlancetaylor commented Jan 25, 2019

ianlancetaylor commented Jan 25, 2019

ianlancetaylor commented Jan 25, 2019

ianlancetaylor commented Jan 25, 2019

cherrymui commented Jan 25, 2019

cherrymui commented Jan 25, 2019

cherrymui commented Jan 25, 2019

laboger commented Jan 25, 2019 via email

ianlancetaylor commented Jan 25, 2019

cherrymui commented Jan 25, 2019

cherrymui commented Jan 25, 2019

ianlancetaylor commented Jan 25, 2019

gopherbot commented Jan 26, 2019

cmd/compile: miscompilation of zero extension on ppc64le #29943

cmd/compile: miscompilation of zero extension on ppc64le #29943

Comments

ianlancetaylor commented Jan 25, 2019

ianlancetaylor commented Jan 25, 2019

ianlancetaylor commented Jan 25, 2019

ianlancetaylor commented Jan 25, 2019

ianlancetaylor commented Jan 25, 2019

cherrymui commented Jan 25, 2019

cherrymui commented Jan 25, 2019

cherrymui commented Jan 25, 2019

laboger commented Jan 25, 2019 via email

ianlancetaylor commented Jan 25, 2019

cherrymui commented Jan 25, 2019

cherrymui commented Jan 25, 2019

ianlancetaylor commented Jan 25, 2019

gopherbot commented Jan 26, 2019