Strange behaviour with loops

[crone123]

What version of Go are you using (go version)?

$ go version
go version go1.16.2 darwin/amd64

(same problem on linux)

Does this issue reproduce with the latest release?

This is the latest release, I mean

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/vcrone/Library/Caches/go-build"
GOENV="/Users/vcrone/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/vcrone/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/vcrone/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/vcrone/sdk/go1.16.2"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/vcrone/sdk/go1.16.2/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.2"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/3b/4rpn6rr54vn1qbqdwvgrv3f80000gn/T/go-build1088450240=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

If you run this code:

func sqrt(x float64) float64 {
	var r float64 = 1
	var old = x
	var first = true
	for first || math.Abs(old - r) > 0.00001 {
		first = false
		old = r
		r -= (r*r - x) / (2*r)
		println("First print")
		println(r)
		println(old)
		println("Second print")
		println(r, old )// This line
		println(r)
		println(old)
	}
	return r
}

Remove this line println(r,old)and the loop fail.

What did you expect to see?

First print
+1.500000e+000
+1.000000e+000
Second print
+1.500000e+000 +1.000000e+000
+1.500000e+000
+1.000000e+000
First print
+1.416667e+000
+1.500000e+000
Second print
+1.416667e+000 +1.500000e+000
+1.416667e+000
+1.500000e+000
First print
+1.414216e+000
+1.416667e+000
Second print
+1.414216e+000 +1.416667e+000
+1.414216e+000
+1.416667e+000
First print
+1.414214e+000
+1.414216e+000
Second print
+1.414214e+000 +1.414216e+000
+1.414214e+000
+1.414216e+000
+1.414214e+000

What did you see instead?

First print
+1.500000e+000
+1.000000e+000
Second print
+1.500000e+000
+1.000000e+000
+1.500000e+000

Additional notes

If I run the same code without optimisations:

go run -gcflags '-N -l' main.go

It works as expected.

Compiler bug?
If this is not a compiler bug, can you explain me what is the problem? Why removing a println cause a total behaviour change?

Thank you

[AlexRouSg]

Please provide a runable example.
sqrt(2) prints the following with println(r,old) removed

First print
+1.500000e+000
+1.000000e+000
Second print
+1.500000e+000
+1.000000e+000

With the println

First print
+1.500000e+000
+1.000000e+000
Second print
+1.500000e+000 +1.000000e+000
+1.500000e+000
+1.000000e+000

Code

package main
import "math"
func sqrt(x float64) float64 {

var r float64 = 1

var old = x

var first = true

for first || math.Abs(old-r) > 0.00001 {

first = false

old = r

r -= (r*r - x) / (2 * r)

println("First print")

println(r)

println(old)

println("Second print")

println(r)

println(old)

}

return r

}
func main() {

sqrt(2)

}

EDIT: Ohhh I see now ... it changes with go run -gcflags '-N -l' .

[AlexRouSg]

cc @randall77

[crone123]

Ok, sorry I forgot the main()

Yes, you see, without -N, -l, the behaviour is not the same with or without the println(r, out)...

[crone123]

I just try to do an exercise from the go tour to get this behaviour..., so I am not an expert in go (that's my question), but the same code compute the good value in some other languages..

[randall77]

I can reproduce, starting with 1.16. Also fails at tip.
Looking at the assembly, the subtraction that is used in the math.Abs(old-r) expression, is SUBSD X0, X0 which can't be right. Initial indications are something wrong with the short circuit pass.

package main

import "math"

func sqrt(x float64) float64 {
	var r float64 = 1
	var old = x
	var first = true
	for first || math.Abs(old-r) > 0.00001 {
		first = false
		old = r
		r -= (r*r - x) / (2 * r)
		println("First print")
		println(r)
		println(old)
		println("Second print")
		println(r, old) // This line
		println(r)
		println(old)
	}
	return r
}
func main() {
	sqrt(2)
}

Strangely, sqrt compiles fine. It's main that has the bug (it contains the inlined body of sqrt).

[randall77]

I think the underlying bug is present in <= 1.15 also. It's just that sqrt is not inlineable before 1.16 (because it has a for loop), so whatever the underlying bug is isn't triggerable with this example.

[randall77]

Here's a simpler reproducer:

package main

//go:noinline
func f(c bool) int {
	b := true
	x := 0
	y := 1
	for b {
		b = false
		y = x
		x = 2
		if c {
			return 3
		}
	}
	return y
}

func main() {
	println(f(false))
}

This reproducer incorrectly prints 2.
It should print 0. And it does, with -gcflags=-d=ssa/short_circuit/off.
The problem is that during short circuit, we don't take into account that the arguments to a phi are from the previous iteration, not the current iteration. That confusion leads to assigning y the current iteration's value of x, not the previous iteration's value of x.

The bug looks like it started in 1.15.

In shortcircuit.go, we need to be a bit more choosy in replaceUses to not replace uses inside a phi, as we're replacing the current iteration's values, not those which are from the previous iteration. At least, something like that; it may not be quite so simple.

@josharian

@gopherbot please open backport issues.

[gopherbot]

Backport issue(s) opened: #45187 (for 1.15).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

[randall77]

@gopherbot please open a backport to 1.16