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

compileopts: adapt GOARM to follow new 1.22 scheme #4189

Closed
wants to merge 2 commits into from

Conversation

MDr164
Copy link

@MDr164 MDr164 commented Mar 13, 2024

This ads the optional softfloat and hardfloat suffix to GOARM the same way it was introduced upstream with Go 1.22. By default armv5 is softfloat while v6 and v7 are hardfloat. Resolves #4177

TODO: Actually figure out the correct combination of LLVM arch flags.

Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to add a new test case to builder/builder_test.go, there's a list of GOOS/GOARCH/GOARM combinations that get tested. Once you do that, you can test these flags using make test. This will tell you the exact LLVM features you need to use.
This currently tests all of TinyGo and takes a while, you can make a modification similar to #4190 to only test the builder package.

compileopts/target.go Outdated Show resolved Hide resolved
compileopts/target.go Outdated Show resolved Hide resolved
@aykevl
Copy link
Member

aykevl commented Mar 13, 2024

It might be useful to try out combinations of flags using Godbolt: https://godbolt.org/z/9cGfzhG6x

@MDr164 MDr164 changed the base branch from release to dev March 26, 2024 13:55
@MDr164 MDr164 force-pushed the goarm-1.22 branch 5 times, most recently from be4f53e to 761b18b Compare March 26, 2024 15:59
@MDr164 MDr164 marked this pull request as ready for review March 26, 2024 16:07
@MDr164
Copy link
Author

MDr164 commented Mar 27, 2024

Looks like CI is failing for code I didn't even touch in this PR. Otherwise this looks fine now. Will have to run a few more tests to be certain all the flags and targets check out on my hardware here but I think it's ready to go.

@MDr164 MDr164 requested a review from aykevl March 27, 2024 08:48
Copy link
Member

@aykevl aykevl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I read this PR correctly, it will now default to softfloat everywhere except when hardfloat is specified explicitly? Is that true? That seems like a bad idea. A better default would be to use softfloat on armv5 and hardfloat on armv6 and armv7 (or softfloat on armv5+armv6 and hardfloat on armv7).

Also, I don't think this affects musl, so you will likely still have some floating point instructions (or ABI incompatibilities). Try running this on an ARM system:

GOARM=5,softfloat tinygo run ./testdata/math.go
GOARM=5,hardfloat tinygo run ./testdata/math.go
GOARM=7,softfloat tinygo run ./testdata/math.go
GOARM=7,hardfloat tinygo run ./testdata/math.go

They should all produce identical output.

compileopts/target.go Outdated Show resolved Hide resolved
This ads the optional softfloat and hardfloat suffix to GOARM
the same way it was introduced upstream with Go 1.22. By default
armv5 is softfloat while v6 and v7 are hardfloat.

Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
Signed-off-by: Marvin Drees <marvin.drees@9elements.com>
@MDr164
Copy link
Author

MDr164 commented Jun 21, 2024

If I read this PR correctly, it will now default to softfloat everywhere except when hardfloat is specified explicitly? Is that true? That seems like a bad idea. A better default would be to use softfloat on armv5 and hardfloat on armv6 and armv7 (or softfloat on armv5+armv6 and hardfloat on armv7).

Also, I don't think this affects musl, so you will likely still have some floating point instructions (or ABI incompatibilities). Try running this on an ARM system:

GOARM=5,softfloat tinygo run ./testdata/math.go
GOARM=5,hardfloat tinygo run ./testdata/math.go
GOARM=7,softfloat tinygo run ./testdata/math.go
GOARM=7,hardfloat tinygo run ./testdata/math.go

They should all produce identical output.

I'll have to run some more tests to make sure this works as expected on musl as well. And yes the idea was that v5 defaults to soft while v6 and v7 default to hard to match upstream behavior. I'll rework this if the musl tests turn out to be what you expect.

aykevl added a commit that referenced this pull request Jul 31, 2024
This adds softfloat support to GOARM, as proposed here:
golang/go#61588

This is similar to #4189 but
with a few differences:

  * It is based on the work to support MIPS softfloat.
  * It fixes the issue that the default changed to softfloat everywhere.
    This PR defaults to softfloat on ARMv5 and hardfloat on ARMv6 and
    ARMv7.
  * It also compiles the C libraries (compiler-rt, musl) using the same
    soft/hard floating point support. This is important because
    otherwise a GOARM=7,softfloat binary could still contain hardware
    floating point instructions.
@aykevl aykevl mentioned this pull request Jul 31, 2024
@MDr164
Copy link
Author

MDr164 commented Jul 31, 2024

Closed in favor of #4374

@MDr164 MDr164 closed this Jul 31, 2024
aykevl added a commit that referenced this pull request Aug 14, 2024
This adds softfloat support to GOARM, as proposed here:
golang/go#61588

This is similar to #4189 but
with a few differences:

  * It is based on the work to support MIPS softfloat.
  * It fixes the issue that the default changed to softfloat everywhere.
    This PR defaults to softfloat on ARMv5 and hardfloat on ARMv6 and
    ARMv7.
  * It also compiles the C libraries (compiler-rt, musl) using the same
    soft/hard floating point support. This is important because
    otherwise a GOARM=7,softfloat binary could still contain hardware
    floating point instructions.
deadprogram pushed a commit that referenced this pull request Aug 14, 2024
This adds softfloat support to GOARM, as proposed here:
golang/go#61588

This is similar to #4189 but
with a few differences:

  * It is based on the work to support MIPS softfloat.
  * It fixes the issue that the default changed to softfloat everywhere.
    This PR defaults to softfloat on ARMv5 and hardfloat on ARMv6 and
    ARMv7.
  * It also compiles the C libraries (compiler-rt, musl) using the same
    soft/hard floating point support. This is important because
    otherwise a GOARM=7,softfloat binary could still contain hardware
    floating point instructions.
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.

Add support for new ARM softfloat/hardfloat distinction added in Go 1.22
2 participants