Skip to content

Conversation

@prashantkhoje
Copy link

This was exposed while testing CockroachDB.
Following tests from pkg/sql/opt/exec/execbuilder fail due to floating point precision differences caused by FMA:

  • TestExecBuild/local/geospatial
  • TestExecBuild/local-vec-off/geospatial
  • TestExecBuild/local-spec-planning/geospatial
  • TestExecBuild/fakedist/geospatial
  • TestExecBuild/fakedist-vec-off/geospatial
  • TestExecBuild/fakedist-metadata/geospatial
  • TestExecBuild/fakedist-disk/geospatial
  • TestExecBuild/fakedist-spec-planning/geospatial

With explicit casts in this patch, these failures are resolved.

References:
https://go.dev/ref/spec#Floating_point_operators
golang/go#53297
golang/go#48145
disintegration/gift#20 (comment)

@prashantkhoje
Copy link
Author

@rsned, please review.

@prashantkhoje
Copy link
Author

@otan, fyi

@prashantkhoje
Copy link
Author

@rsned
Copy link
Collaborator

rsned commented Apr 21, 2023

Can you see if this still occurs with the latest updates.

@prashantkhoje
Copy link
Author

Yes. Cockroachdb still encounters FMA failures with the latest golang/geo.
The changes proposed here fixes the deviation.

This was exposed while testing CockroachDB.
Following tests from pkg/sql/opt/exec/execbuilder fail due to floating point precision differences caused by FMA:
- TestExecBuild/local/geospatial
- TestExecBuild/local-vec-off/geospatial
- TestExecBuild/local-spec-planning/geospatial
- TestExecBuild/fakedist/geospatial
- TestExecBuild/fakedist-vec-off/geospatial
- TestExecBuild/fakedist-metadata/geospatial
- TestExecBuild/fakedist-disk/geospatial
- TestExecBuild/fakedist-spec-planning/geospatial

With explicit casts in this patch, these failures are resolved.

References:
https://go.dev/ref/spec#Floating_point_operators
golang/go#53297
golang/go#48145
disintegration/gift#20 (comment)
@jmr jmr force-pushed the ppc64le-cdb-execbuilder branch from 153cf68 to 79b2dce Compare April 1, 2025 08:06
@jmr
Copy link
Collaborator

jmr commented Apr 1, 2025

Does go have anything like FP_CONTRACT OFF?

https://learn.microsoft.com/en-us/cpp/preprocessor/fp-contract?view=msvc-170

https://github.com/google/s2geometry/blob/58de4ea1e2f8a294e0c072c602c22232fd1433ad/src/s2/_fp_contract_off.h#L43

It seems we'll be playing whack-a-mole otherwise.

There is cpu.fma=off, but I'm not really sure what it does: golang/go#36971

@rsned
Copy link
Collaborator

rsned commented Apr 1, 2025

We've done some whack-a-mole previously which did resolve some of them. I know nothing about fiddling with compiler optimization settings in Go. This one is simple enough to add, but I would say any future ones would need more convincing or figuring out the right set of Go build flags to embed in the file.

@prashantkhoje
Copy link
Author

This change makes the code more portable across platforms without dependency on the compiler.

@jmr
Copy link
Collaborator

jmr commented Dec 30, 2025

I didn't find any go option to disable FMA, so I guess this is as good as it gets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants