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.

Sign up for GitHub

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

runtime: mkfastlog2table produces different output on darwin/arm64 #49891

Closed
josharian opened this issue Dec 1, 2021 · 11 comments · Fixed by ferrmin/go#404
Closed

runtime: mkfastlog2table produces different output on darwin/arm64 #49891

josharian opened this issue Dec 1, 2021 · 11 comments · Fixed by ferrmin/go#404
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.20

Comments

@josharian
Copy link
Contributor

On darwin/arm64 at 3ca57c7 (Nov 29, 2021), running go generate in package runtime generates a diff:

diff --git a/src/runtime/fastlog2table.go b/src/runtime/fastlog2table.go
index 6ba4a7d3f2..d8bb953939 100644
--- a/src/runtime/fastlog2table.go
+++ b/src/runtime/fastlog2table.go
@@ -8,22 +8,22 @@ const fastlogNumBits = 5
 
 var fastlog2Table = [1<<fastlogNumBits + 1]float64{
 	0,
-	0.0443941193584535,
-	0.08746284125033943,
-	0.12928301694496647,
-	0.16992500144231248,
-	0.2094533656289499,
+	0.044394119358453485,
+	0.08746284125033939,
+	0.12928301694496644,
+	0.16992500144231246,
+	0.20945336562894987,
 	0.24792751344358555,
 	0.28540221886224837,
-	0.3219280948873623,
-	0.3575520046180837,
-	0.39231742277876036,
-	0.4262647547020979,
-	0.4594316186372973,
-	0.4918530963296748,
+	0.32192809488736235,
+	0.35755200461808373,
+	0.3923174227787603,
+	0.42626475470209796,
+	0.45943161863729726,
+	0.4918530963296747,
 	0.5235619560570128,
 	0.5545888516776374,
-	0.5849625007211563,
+	0.5849625007211562,
 	0.6147098441152082,
 	0.6438561897747247,
 	0.6724253419714956,
@@ -31,7 +31,7 @@ var fastlog2Table = [1<<fastlogNumBits + 1]float64{
 	0.7279204545631992,
 	0.7548875021634686,
 	0.7813597135246596,
-	0.8073549220576042,
+	0.8073549220576041,
 	0.8328900141647417,
 	0.8579809951275721,
 	0.8826430493618412,
diff --git a/src/runtime/preempt_mips64x.s b/src/runtime/preempt_mips64x.s
index 996b592ae0..99e680c39e 100644
--- a/src/runtime/preempt_mips64x.s
+++ b/src/runtime/preempt_mips64x.s
@@ -1,7 +1,6 @@
 // Code generated by mkpreempt.go; DO NOT EDIT.
 
 //go:build mips64 || mips64le
-
 #include "go_asm.h"
 #include "textflag.h"
 
diff --git a/src/runtime/preempt_mipsx.s b/src/runtime/preempt_mipsx.s
index 7b169acd99..6d808813d1 100644
--- a/src/runtime/preempt_mipsx.s
+++ b/src/runtime/preempt_mipsx.s
@@ -1,7 +1,6 @@
 // Code generated by mkpreempt.go; DO NOT EDIT.
 
 //go:build mips || mipsle
-
 #include "go_asm.h"
 #include "textflag.h"
 
diff --git a/src/runtime/preempt_ppc64x.s b/src/runtime/preempt_ppc64x.s
index 2c4d02edfe..66c33276ec 100644
--- a/src/runtime/preempt_ppc64x.s
+++ b/src/runtime/preempt_ppc64x.s
@@ -1,7 +1,6 @@
 // Code generated by mkpreempt.go; DO NOT EDIT.
 
 //go:build ppc64 || ppc64le
-
 #include "go_asm.h"
 #include "textflag.h"

Should we regenerate and commit prior to the release?
@josharian josharian added this to the Go1.18 milestone Dec 1, 2021
@robpike
Copy link
Contributor

robpike commented Dec 1, 2021 •
edited

Is this a change due to registerization of arguments? Or some other optimization? It's just the last digit changing, but it would be nice to understand why.

@rsc
Copy link
Contributor

rsc commented Dec 1, 2021 •
edited

Why did it delete the blank lines? That's definitely a bug and not a math problem.

I see that's a different generator. That one's easy to fix.

@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

We don't need to regenerate and commit. The table is only used to get 5 digits of log.

It is still interesting to find out why arm64 produces slightly different numbers. We have excised all the 387 intrinsics, so that's not it. I thought maybe the x86-64 asm was producing a different answer from the pure Go that arm64 would use, but setting haveArchLog = false in math/log_asm.go on x86-64 did not change the behavior of mkfastlog2table.

At that point I think it's all pure Go code - Log2 calls Log calls Frexp, and only Log had any assembly (on x86-64). It's a bit concerning but not enough to block the release.

@rsc rsc changed the title runtime: 'go generate' makes changes runtime: mkfastlog2table produces different output on darwin/arm64 Dec 1, 2021
@randall77
Copy link
Contributor

Could be FMA on arm64. mkfastlog2table doesn't have any FMA issues, but maybe there are FMA candidates in math.Log2?

@rsc
Copy link
Contributor

rsc commented Dec 1, 2021

log2 itself is a single FMA. Could be.

@randall77
Copy link
Contributor

I've verified that disabling FMA in math.log2 (adding a float64 cast) fixes the precision differences.

@josharian
Copy link
Contributor Author

Phew. So now it's just a question of finding a way to make the generator programs stable and match the existing runtime directory.

@randall77
Copy link
Contributor

I think the easiest thing would be just to copy log2 out of math and into mkfastlog2table and add the float64 cast that disables FMA.

@mknyszek
Copy link
Contributor

mknyszek commented Dec 2, 2021

This doesn't seem like it's necessarily a new issue in this release, and it certainly shouldn't block it. Moving this to the 1.19 milestone for now.

@mknyszek mknyszek added the NeedsFix The path to resolution is known, but the work has not been done. label Dec 2, 2021
@mknyszek mknyszek modified the milestones: Go1.18, Go1.19 Dec 2, 2021
@ianlancetaylor
Copy link
Contributor

Rolling forward to 1.20. Please comment if you disagree. Thanks.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.19, Go1.20 Jun 24, 2022
@gopherbot
Copy link

Change https://go.dev/cl/413976 mentions this issue: runtime: avoid fma in mkfastlog2table

@ferrmin ferrmin mentioned this issue Jun 27, 2022
Merged
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@ianlancetaylor @jproberts
runtime: avoid fma in mkfastlog2table
84087aa 
This lets us generate identical copies of fastlog2table.go on all hosts.

Tested by regenerating fastlog2table.go on linux-amd64 and darwin-arm64.

Fixes golang#49891

Change-Id: I279d6b5abb5a5290c049d9658050fd9c8d0c0190
Reviewed-on: https://go-review.googlesource.com/c/go/+/413976
Run-TryBot: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@golang golang locked and limited conversation to collaborators Jun 26, 2023
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.
Projects
Release Status & Release Blockers
Status: Done
Milestone
Go1.20
Development

Successfully merging a pull request may close this issue.

runtime: avoid fma in mkfastlog2table golang/go
7 participants
@josharian @rsc @mknyszek @ianlancetaylor @robpike @randall77 @gopherbot