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

bug: Compilation error on new AMD platform #56

Closed
NagyZoltanPeter opened this issue Apr 10, 2024 · 10 comments · Fixed by #58
Closed

bug: Compilation error on new AMD platform #56

NagyZoltanPeter opened this issue Apr 10, 2024 · 10 comments · Fixed by #58
Assignees

Comments

@NagyZoltanPeter
Copy link

While building nim-waku we faced an issue compiling secp256k1 on a new AMD Ryzen 7 PRO 7840U system (on UBUNTU 22.04 LTS)
waku-org/nwaku#2560

originally reported from outside

I would like to propose a quick workaround until it maybe fixed in the upstream library, otherwise please advise.
patch: amd-asm-compilation-fix

@NagyZoltanPeter NagyZoltanPeter self-assigned this Apr 12, 2024
@mratsim
Copy link
Contributor

mratsim commented Apr 12, 2024

Can you check with Clang?

Also can you ensure that LTO is not used for crypto: https://github.com/status-im/nimbus-eth2/blob/dc19b08/config.nims#L220-L221, as if there is inlining, the compiler might run out of registers.

You might also want to ensure you have enough register by passing -fomit-frame-pointer to secp256k1 building.
Cryptographic libraries need all the registers they can get, and not passing this will incure a large performance penalty:

https://www.phoronix.com/review/fedora-frame-pointer/2

image

@NagyZoltanPeter
Copy link
Author

Can you check with Clang?

Also can you ensure that LTO is not used for crypto: https://github.com/status-im/nimbus-eth2/blob/dc19b08/config.nims#L220-L221, as if there is inlining, the compiler might run out of registers.

You might also want to ensure you have enough register by passing -fomit-frame-pointer to secp256k1 building. Cryptographic libraries need all the registers they can get, and not passing this will incure a large performance penalty:

https://www.phoronix.com/review/fedora-frame-pointer/2

image

Certainly, but it may not help find out the issue.
I have no access to that type of AMD cpu powered machine that produced the error we faced. My colleague has that one. On my Intel with gcc 12 on Ubuntu I can compile even the asm inline part, tests are just seems fine.
Also another issue that in nim-waku we compilee with gcc so even clang would compile it it may not help.
What I can do in addition to this workaround to point our dependency to a separate branch here and thus to avoid main line would have this forced avoid of that specific asm inline code.
But I'm curious if other project depending on this version of nim-secp256k1 - I suppose - would fail to compile in similar cirtumstances.

@tersec
Copy link
Contributor

tersec commented Apr 12, 2024

Oppose this "quick workaround" from landing in this library. It would come with visible perf regressions for nimbus-eth2.

@tersec
Copy link
Contributor

tersec commented Apr 12, 2024

Certainly, but it may not help find out the issue. I have no access to that type of AMD cpu powered machine that produced the error we faced. My colleague has that one.

This should be reproducible by specifying the correct -march flags to the correct gcc version (which has been specified above), even if one doesn't have said CPU, as it will "cross-compile" of sorts regardless.

@NagyZoltanPeter
Copy link
Author

Thanks for the tips. I accept the perf concerns.
Will try to come back with more proper analysis and/or issue a ticket of possible on the vendor repo.
I'm not sure about nimbus, but waku supposed to be run on different platforms, hence the workaround till can have fix. So I can imagine a solution we can use that does not influence other users.

@mratsim
Copy link
Contributor

mratsim commented Apr 14, 2024

Certainly, but it may not help find out the issue.
I have no access to that type of AMD cpu powered machine that produced the error we faced. My colleague has that one. On my Intel with gcc 12 on Ubuntu I can compile even the asm inline part, tests are just seems fine.

The issue is about "error: ‘asm’ operand has impossible constraints" which means that GCC fails to find enough registers. If you compile with Clang and it fails you will have a better error message.

Every step, I outline will help you pinpoint the issue, this is something I dealt with on a regular basis:
remove this -fomit-frame-pointer from my assembly code and the compiler will throw the "impossible constraints" error: https://github.com/mratsim/constantine/blob/976c8bb/constantine/math/arithmetic/assembly/limbs_asm_modular_x86.nim#L23-L24

Also the issue is not AMD specific, there is no AMD vs Intel, all x86-64 CPUs have the same compilation path. You need the same Ubuntu version, Ubuntu changed the defaults of GCC on 24.04. If your colleague is using a preview version, it's normal they have the issue and not you.

https://www.phoronix.com/forums/forum/software/distributions/1428123-ubuntu-24-04-lts-to-enable-frame-pointers-by-default-for-better-profiling-debugging

If you want to test on your machine, use --passC:-fno-omit-frame-pointer and confirm whether you get the same error message or not.

@tersec
Copy link
Contributor

tersec commented Jun 3, 2024

@mratsim nimbus-eth2 explicitly enables frame pointers:

# omitting frame pointers in nim breaks the GC
# https://github.com/nim-lang/Nim/issues/10625
switch("passC", "-fno-omit-frame-pointer")
switch("passL", "-fno-omit-frame-pointer")

to work around another Nim issue.

@tersec
Copy link
Contributor

tersec commented Jun 3, 2024

For anyone experiencing this from nwaku, nimbus-eth1, or nimbus-eth2: status-im/nimbus-eth2#6324 (comment) describes how to find what -march=native resolves to. I've tried repro'ing this with

# gcc --version
gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0

by explicitly selecting either -march=znver2 or -march=znver3 by docker pull ubuntu:22.04, so there's some other necessary condition.

nwaku also uses -march=native by default as does nimbus-eth1 (which also explicitly enables frame pointers for the same reason as nimbus-eth2).

@mratsim
Copy link
Contributor

mratsim commented Jul 3, 2024

Confirmed upstream, the library should be compiled with -fomit-frame-pointer otherwise GCC runs out of registers: bitcoin-core/secp256k1#846 (comment)

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 a pull request may close this issue.

3 participants