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

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

Closed
ianlancetaylor opened this issue Jan 25, 2019 · 13 comments

Comments

Projects
None yet
4 participants
@ianlancetaylor
Copy link
Contributor

commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

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

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

CC @rasky

@cherrymui

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Jan 25, 2019

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

This comment has been minimized.

Copy link

commented Jan 26, 2019

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

@gopherbot gopherbot closed this in d585f04 Jan 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.