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: wrong code generation #15175

Closed
brtzsnr opened this issue Apr 7, 2016 · 9 comments
Closed

cmd/compile: wrong code generation #15175

brtzsnr opened this issue Apr 7, 2016 · 9 comments
Milestone

Comments

@brtzsnr
Copy link
Contributor

brtzsnr commented Apr 7, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
    go version devel +121c434 Thu Apr 7 09:57:06 2016 +0000 linux/amd64
  2. What operating system and processor architecture are you using (go env)?
    linux amd64
  3. What did you do?

Compiled and ran the following code:

// gostress -seed 22885316 -want 253
package main
import "fmt"
import "os"
var b2i = map[bool]int{true:1}
func main() {
if got := f1_ssa(0, false, false, 1, 2, 2, 1, 1, ); got != 253 {
fmt.Printf("f1_ssa(0, false, false, 1, 2, 2, 1, 1, ) = %v, wanted 253\n", got)
os.Exit(1)
}
}
func f1_ssa(a1 uint, a2 bool, a3 bool, a4 uint64, a5 int8, a6 uint8, a7 int, a8 uint64, ) uint8 {
switch {} // prevent inlining
a4--
a7 *= a7 // int
a6 -= a6 + 3 // uint8
v1 := a7 | ((a7 & a7) + (a7 << a6) | (a7 - a7 - a7)) // int
_ = v1
a7--
v2 := a4 - (a4 & a4) // uint64
_ = v2
v3 := a3 // bool
_ = v3
return a6 >> (((2 << 3) - 1) >> a6)
}
  1. What did you expect to see?
    To print nothing
  2. What did you see instead?
f1_ssa(0, false, false, 1, 2, 2, 1, 1, ) = 253, wanted 253
exit status 1

Clearly the comparison is wrong here.

@ALTree
Copy link
Member

ALTree commented Apr 7, 2016

Small comment:

switch {} // prevent inlining

this no longer works. In fact, at tip f1_ssa is inlined.

@ALTree
Copy link
Member

ALTree commented Apr 7, 2016

Also, disabling inlining fixes the issue: the code prints nothing.

@brtzsnr
Copy link
Contributor Author

brtzsnr commented Apr 7, 2016

Happy accident. The fact that the code behaves differently with inlining disabled and enabled it's a clear bug in the code generated. Most likely it's a rule that folds the comparisons, but I'd have to minimize the code first to figure out which. Alternatively one can bisect and find which patch is the culprit.

@brtzsnr
Copy link
Contributor Author

brtzsnr commented Apr 7, 2016

@bradfitz bradfitz added this to the Go1.7 milestone Apr 7, 2016
@brtzsnr
Copy link
Contributor Author

brtzsnr commented Apr 7, 2016

This is one is minimized:

package main
import "fmt"
import "os"
func main() {
a6 := uint8(253)
got := a6 >> 0
if got != 253 {
fmt.Printf("f1_ssa(0, false, false, 1, 2, 2) = %v, wanted 253\n", got)
os.Exit(1)
}
}

@ALTree
Copy link
Member

ALTree commented Apr 7, 2016

Git bisect says it was introduced in https://go-review.googlesource.com/#/c/21256/

@brtzsnr
Copy link
Contributor Author

brtzsnr commented Apr 7, 2016

There are a few things wrong:
a6 := uint8(253) -> (Const8 [-3]) . I think it should be (Const8 253) instead.

This also happens:
(Neq8 (Const8 [-3]) (Const8 [-3])) -> (ConstBool [true])

which is an instantiation of
(Neq8 (Const8 [c]) (Const8 [d])) -> (ConstBool [b2i(c != d)])

where c = 253 (coming from right shift) and d = -3 (coming from the constant above).

@randall77

@randall77
Copy link
Contributor

In 21256 I changed the semantics of the AuxInt field to always be sign-extended. So -3 is right, 253 is wrong. It is ending up with 253 anyway because of a bad rewrite rule. CL pending.

@gopherbot
Copy link

CL https://golang.org/cl/21586 mentions this issue.

@golang golang locked and limited conversation to collaborators Apr 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants