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

Include copy of fmath with fix for archs other than amd64 #28

Merged
merged 1 commit into from
May 20, 2021

Conversation

hubues
Copy link
Contributor

@hubues hubues commented May 18, 2021

Imported package github.com/barnex/fmath does not compile on other architectures than amd64:

$ GOOS=linux GOARCH=arm go build
# github.com/barnex/fmath
/Users/hugues/go/pkg/mod/github.com/barnex/fmath@v0.0.0-20150108074215-ec9671f295c2/sqrtf_decl.go:7:6: missing function body

fmath package provides float32 wrappers for standard math package float64 functions. It as (only) one optimization in assembly code for sqrt function, for architecture amd64. This optimization is implemented in two files: sqrtf_amd64.s and sqrtf_decl.go. That later file must be compiled only for amd64 arch because its function body is empty and implemented in sqrtf_amd64.s. Currently it is included in builds for all archs, leading to the error reported above. To fix that, it should be renamed or include a build flag:

// +build amd64

This pull request proposes to include (copy) fmath package code from github.com/barnex/fmath into go3d, and remove the dependency to github.com/barnex/fmath. One could argue that github.com/barnex/fmath could be fixed instead, but that project shows no activity since 6 years. In addition, it is very simply made of generated wrapper code from a simple bash script (see codegen.bash). The only optimisation is within sqrtf function, that works only for amd64.
Hence a pragmatic approach is to embed the fixed fmath functions into go3d (just my humble opinion).

@ungerik ungerik merged commit 9ffa86f into ungerik:master May 20, 2021
@ungerik
Copy link
Owner

ungerik commented May 20, 2021

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants