Navigation Menu

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

Jump to bottom

cmd/compile: uint64(Float32bits(f)) on amd64 doesn't clear high bits #25322

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

cmd/compile: uint64(Float32bits(f)) on amd64 doesn't clear high bits #25322

vishnulzr opened this issue May 10, 2018 · 12 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.11

Comments

@vishnulzr
Copy link

vishnulzr commented May 10, 2018 •
edited by agnivade

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
Copy link
Contributor

davecheney commented May 10, 2018 •
edited

I get

c040000000

@vishnulzr
Copy link
Author

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 10, 2018
@agnivade agnivade added this to the Go1.11 milestone May 10, 2018
@agnivade
Copy link
Contributor

/cc @TocarIP @josharian

@josharian
Copy link
Contributor

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

@ALTree
Copy link
Member

ALTree 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
Copy link
Contributor

cznic 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
Copy link
Member

ALTree 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
Copy link
Member

ALTree commented May 10, 2018 •
edited

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
Copy link
Contributor

Thanks, @ALTree.

cc @randall77

This is definitely a backport candidate.

@josharian josharian added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 10, 2018
@gopherbot
Copy link

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

@josharian
Copy link
Contributor

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

@gopherbot gopherbot mentioned this issue May 10, 2018
Closed
@gopherbot
Copy link

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.

@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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
7 participants
@davecheney @josharian @cznic @agnivade @ALTree @gopherbot @vishnulzr