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: uint64(Float32bits(f)) on amd64 doesn't clear high bits #25322

Closed
vishnulzr opened this issue May 10, 2018 · 12 comments

Comments

Projects
None yet
7 participants
@vishnulzr
Copy link

commented May 10, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version go1.10.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"

happens on GOOS="linux"

What did you do?

the high 32 bits of uint64(math.Float32bits(x)) are not zero

If possible, provide a recipe for reproducing the error.

package main

import (
	"fmt"
	"math"
)

func Foo(v float32) {
	fmt.Printf("%x\n", uint64(math.Float32bits(v)))
}

func main() {
	Foo(2.0)
}

this produces "c440000000"

What did you expect to see?

40000000

What did you see instead?

c40000000

The high order bits are not cleared in the cast to uint64

works fine in 1.9.6 and the generated code in 1.10.2 seems to optimize the cast out.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented May 10, 2018

I get

c040000000
@vishnulzr

This comment has been minimized.

Copy link
Author

commented May 10, 2018

Which is not the right value.

The MSB 32 bits should be zero

@robpike robpike changed the title uint64 cast of Float32bits cmd/compiler: uint64(Float32bits(f)) on amd64 doesn't clear high bits May 10, 2018

@agnivade agnivade changed the title cmd/compiler: uint64(Float32bits(f)) on amd64 doesn't clear high bits cmd/compile: uint64(Float32bits(f)) on amd64 doesn't clear high bits May 10, 2018

@agnivade agnivade added this to the Go1.11 milestone May 10, 2018

@agnivade

This comment has been minimized.

Copy link
Member

commented May 10, 2018

@josharian

This comment has been minimized.

Copy link
Contributor

commented May 10, 2018

Thanks! I’m AFK for a bit. If anyone has time to bisect that’d be helpful.

@ALTree

This comment has been minimized.

Copy link
Member

commented May 10, 2018

Git bisect points to 229aaac. Not sure if that makes sense...

$ git checkout 229aaac19e041ac74ab043d6ef09c8406bb0a9e7
Note: checking out '229aaac19e041ac74ab043d6ef09c8406bb0a9e7'.

HEAD is now at 229aaac19e runtime: remove getcallerpc argument
$ (./make.bash 2>&1) > /dev/null
$ gotip run ~/Desktop/test.go
c440000000

$ git checkout 229aaac19e041ac74ab043d6ef09c8406bb0a9e7^
Previous HEAD position was 229aaac19e runtime: remove getcallerpc argument
HEAD is now at 8cb2952f2f os/exec: remove protection against simultaneous Wait/Write
$ (./make.bash 2>&1) > /dev/null
$ gotip run ~/Desktop/test.go
40000000
@cznic

This comment has been minimized.

Copy link
Contributor

commented May 10, 2018

Not sure if that makes sense...

My guess is that on a different machine/OS/platform bisect may point to a different commit.

@ALTree

This comment has been minimized.

Copy link
Member

commented May 10, 2018

Yeah, scratch that, the generated code for Foo is the same in those two commits. Bisecting on the program output was a mistake, I should probably bisect on the difference in the generated asm.

@ALTree

This comment has been minimized.

Copy link
Member

commented May 10, 2018

Ok, so after fb05948 the generated asm changed from

old:

"".Foo STEXT size=181 args=0x8 locals=0x68
	0x0000 00000 (test.go:8)	TEXT	"".Foo(SB), $104-8
	0x0000 00000 (test.go:8)	MOVQ	(TLS), CX
	0x0009 00009 (test.go:8)	CMPQ	SP, 16(CX)
	0x000d 00013 (test.go:8)	JLS	171
	0x0013 00019 (test.go:8)	SUBQ	$104, SP
	0x0017 00023 (test.go:8)	MOVQ	BP, 96(SP)
	0x001c 00028 (test.go:8)	LEAQ	96(SP), BP
	0x0021 00033 (test.go:8)	FUNCDATA	$0, gclocals·263043c8f03e3241528dfae4e2812ef4(SB)
	0x0021 00033 (test.go:8)	FUNCDATA	$1, gclocals·e226d4ae4a7cad8835311c6a4683c14f(SB)
	0x0021 00033 (test.go:8)	MOVSS	"".v+112(SP), X0
	0x0027 00039 (test.go:9)	MOVSS	X0, math.f·2+68(SP)
	0x002d 00045 (test.go:9)	MOVL	math.f·2+68(SP), AX
	0x0031 00049 (test.go:9)	MOVQ	AX, ""..autotmp_4+72(SP)
	0x0036 00054 (test.go:9)	XORPS	X0, X0
	0x0039 00057 (test.go:9)	MOVUPS	X0, ""..autotmp_3+80(SP)
	0x003e 00062 (test.go:9)	LEAQ	type.uint64(SB), AX
	0x0045 00069 (test.go:9)	MOVQ	AX, (SP)
	0x0049 00073 (test.go:9)	LEAQ	""..autotmp_4+72(SP), AX
	0x004e 00078 (test.go:9)	MOVQ	AX, 8(SP)
	0x0053 00083 (test.go:9)	PCDATA	$0, $1
	0x0053 00083 (test.go:9)	CALL	runtime.convT2E64(SB)
	[...]

to

new:

"".Foo STEXT size=170 args=0x8 locals=0x60
	0x0000 00000 (test.go:8)	TEXT	"".Foo(SB), $96-8
	0x0000 00000 (test.go:8)	MOVQ	(TLS), CX
	0x0009 00009 (test.go:8)	CMPQ	SP, 16(CX)
	0x000d 00013 (test.go:8)	JLS	160
	0x0013 00019 (test.go:8)	SUBQ	$96, SP
	0x0017 00023 (test.go:8)	MOVQ	BP, 88(SP)
	0x001c 00028 (test.go:8)	LEAQ	88(SP), BP
	0x0021 00033 (test.go:8)	FUNCDATA	$0, gclocals·263043c8f03e3241528dfae4e2812ef4(SB)
	0x0021 00033 (test.go:8)	FUNCDATA	$1, gclocals·e226d4ae4a7cad8835311c6a4683c14f(SB)
	0x0021 00033 (test.go:9)	MOVQ	"".v+104(SP), AX
	0x0026 00038 (test.go:9)	MOVQ	AX, ""..autotmp_4+64(SP)
	0x002b 00043 (test.go:9)	XORPS	X0, X0
	0x002e 00046 (test.go:9)	MOVUPS	X0, ""..autotmp_3+72(SP)
	0x0033 00051 (test.go:9)	LEAQ	type.uint64(SB), AX
	0x003a 00058 (test.go:9)	MOVQ	AX, (SP)
	0x003e 00062 (test.go:9)	LEAQ	""..autotmp_4+64(SP), AX
	0x0043 00067 (test.go:9)	MOVQ	AX, 8(SP)
	0x0048 00072 (test.go:9)	PCDATA	$0, $1
	0x0048 00072 (test.go:9)	CALL	runtime.convT2E64(SB)
	[...]

This sequence:

	0x0021 00033 (test.go:8)	MOVSS	"".v+112(SP), X0
	0x0027 00039 (test.go:9)	MOVSS	X0, math.f·2+68(SP)
	0x002d 00045 (test.go:9)	MOVL	math.f·2+68(SP), AX

disappeared, and was replaced by a single MOVQ "".v+104(SP), AX.

@josharian

This comment has been minimized.

Copy link
Contributor

commented May 10, 2018

Thanks, @ALTree.

cc @randall77

This is definitely a backport candidate.

@gopherbot

This comment has been minimized.

Copy link

commented May 10, 2018

Change https://golang.org/cl/112637 mentions this issue: cmd/compile: fix zero extend after float->int conversion

@josharian

This comment has been minimized.

Copy link
Contributor

commented May 10, 2018

@gopherbot please consider this for backport to 1.10, it results in incorrect code generation.

@gopherbot

This comment has been minimized.

Copy link

commented May 10, 2018

Backport issue(s) opened: #25335 (for 1.10).

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

@gopherbot gopherbot closed this in 599f56d May 10, 2018

@golang golang locked and limited conversation to collaborators May 10, 2019

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