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

Rework LTO, Intel Assembly syntax and default compile-options #309

Merged
merged 8 commits into from
Dec 2, 2023

Conversation

mratsim
Copy link
Owner

@mratsim mratsim commented Dec 2, 2023

This tries to address #303 #295 codex-storage/nim-codex#625 (comment) following making Constantine compatible with LTO #231

  • Intel assembly is used on a per-file basis to avoid interfering with other libraries like secp256k1 doesn't work
  • On Apple MacOS/iOS it will need to be explicitly enabled, due to:
    • Clang not supporting Intel syntax:
    • Intel Apple CPUs being outdated and not being used for high-performance cryptography and Intel syntax being irrelevant for ARM CPUs.

@mratsim
Copy link
Owner Author

mratsim commented Dec 2, 2023

MacOS is so annoying ....

  • Apple Clang without LTO, removes some global variables (ADX) when compiling to a static library
  • LLVM Clang v15 with -flto=thin / -d:lto_incremental does not find enough registers .... but it does with full LTO
  • And Apple Clang not supporting Intel syntax

@mratsim
Copy link
Owner Author

mratsim commented Dec 2, 2023

@dryajov @benbierens @markspanbroek

Constantine now uses Intel Syntax only if LTO is chosen instead of always, hence it isn't needed to set it in libraries that depend on it.

const UseAsmSyntaxIntel* {.booldefine.} = defined(lto) or defined(lto_incremental)
## When using LTO with AT&T syntax
## - GCC will give spurious "Warning: missing operand; zero assumed" with AT&T syntax
## - Clang will wrongly combine memory offset and constant propagated address of constants
##
## Intel syntax does not have such limitation.
## However
## - It is not supported on Apple Clang due to missing
## commit: https://github.com/llvm/llvm-project/commit/ae98182cf7341181e4aa815c372a072dec82779f
## Revision: https://reviews.llvm.org/D113707
## Apple bug: FB12137688
## - Global "-masm=intel" composition with other libraries that use AT&T inline assembly
##
## As a workaround:
## - On MacOS/iOS upstream Clang can be used instead of Apple fork.
## - Do not use LTO or build Constantine as a separate library
##
## Regarding -masm=intel:
## - It might be possible to use Intel assembly is used on a per-file basis
## so that we do not affect other libraries that might be compiled together with Constantine.
## Generating an object file works but the final linking step assumes AT&T syntax and fails.
## - Surrounding code with ".intel_syntax noprefix" and "att_syntax prefix"
## doesn't work with memory operands.
when UseAsmSyntaxIntel:
{.passC: "-masm=intel".}

If using LTO:

  • Intel syntax is necessary for Clang, recommended for GCC (or it will spam spurious warnings that may denial-of-service the console) and so activated by default.
  • Dynamic linking solves assembly syntax incompatibility with other libraries like libsecp256k1
  • On MacOS, Intel Syntax requires upstream LLVM Clang, not Apple Clang
  • If static linking, using the same compiler and the same major version is strongly recommended

@mratsim mratsim merged commit a810179 into master Dec 2, 2023
16 checks passed
@mratsim mratsim deleted the gcc-asm branch December 2, 2023 17:58
mratsim added a commit that referenced this pull request Dec 2, 2023
mratsim added a commit that referenced this pull request Dec 4, 2023
* zal: export MSM and threadpool to C DLL

* fix LTO spurious warnings when building library

* fix appType staticLib vs staticlib

* fix threadpool

* shorter EC FFI

* zal: now compiles with halo2curves and can call Nim

* zal: Pass tests

* zal: add benches Halo2curves vs Constantine

* CI: add constantine-rust / ZAL to CI

* CI: can't use variables for shell

* CI: display rust version

* Rust: force clang for building (Rust regression following #309 removal of rustBuild in .nimble)

* LLVM (hence llvm-config) isn't installed in Github Actions CI

* documentation of compile options

* reduce rust verbosity

* cargo in CI, spray and pray

* don't update Rust twice

* if using clang, Nim expects llvm-ar for static libraries archiver

* Expose full LLVM on MacOS, no Rust for i386, CPU features

* fix HW info

* the light at the end of the CI tunnel

* ensure autoloading kernel32.dll threading API

* Keep stack guard pages on Windows

* MacOS CI: some agents don't support ADX instructions and SIGILL
markspanbroek added a commit to vacp2p/nim-libp2p that referenced this pull request Dec 11, 2023
Fixes assembly incompatibility with secp256k1:
mratsim/constantine#309
dryajov pushed a commit to vacp2p/nim-libp2p that referenced this pull request Dec 22, 2023
Fixes assembly incompatibility with secp256k1:
mratsim/constantine#309
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant