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

Rust build.rs: Pass -D__BLST_NO_ASM__ on non-x86-64/non-aarch64 ISAs #398

Closed
mratsim opened this issue Feb 23, 2024 · 7 comments
Closed

Comments

@mratsim
Copy link

mratsim commented Feb 23, 2024

The current build.rs does not pass the -D__BLST_NO_ASM__ flag when compiling for non-x86-64 / non-aarch64 ISAs.

https://github.com/supranational/blst/blob/0d46eefa45fc1e57aceb42bba0e84eab3a7a9725/build.sh#L91-L93

This is impacting zkVMs that target other ISAs like Risc-V, WASM, MIPS or plain LLVM IR.

Note: ultimately zkVMs will optimize blob proof verification using:

but this will still be helpful for testing and benchmarking.

@jtraglia
Copy link
Member

Hey @mratsim, is this problem specific to the Rust bindings? Would you be willing to make a PR for this?

@jtraglia
Copy link
Member

Hey again. I made a quick PR which I think will handle this. Do you have a system to test this with?

@mratsim
Copy link
Author

mratsim commented Feb 23, 2024

Other platforms might also need something similar if they use a package manager and integrate building C libraries, like Go and Nim.

I will make a PR.

@mratsim
Copy link
Author

mratsim commented Feb 23, 2024

After investigating some more, note that zkVM of interest use Risc-V 32-bit instruction, see cargo toolchain update: rust-lang/rust#117958.

The compiler target-triple (an LLVM abstraction) is riscv32im-risc0-zkvm-elf

BLST C/ASM

According to BLST, when the pointer is of size 4 bytes / 32-bit, no assembly should be pulled:

https://github.com/supranational/blst/blob/0d46eefa45fc1e57aceb42bba0e84eab3a7a9725/build/assembly.S#L136-L139

#elif defined(__BLST_NO_ASM__) || \
      (defined(__SIZEOF_POINTER__) && __SIZEOF_POINTER__==4)
/* inaccurate way to detect a 32-bit processor, but it's close enough */
#else

And within the code, the fallback to no_asm.h is triggered by __BLST_NO_ASM__ which is automatically set for 32-bit platforms:
https://github.com/supranational/blst/blob/0d46eefa45fc1e57aceb42bba0e84eab3a7a9725/src/vect.h#L28-L36

#else                   /* 32 bits on 32-bit platforms, 64 - on 64-bit */
typedef unsigned long limb_t;
#  ifdef _LP64
#   define LIMB_T_BITS   64
#  else
#   define LIMB_T_BITS   32
#   define __BLST_NO_ASM__
#  endif
#endif

I conclude that for some reason the rust toolchain does not report the pointer size properly or maybe there is cross-compilation mixup somewhere.

BLST Go

Looking at BLST Go further confirm my suspicions on 32-bit platforms: https://github.com/supranational/blst/blob/0d46eefa45fc1e57aceb42bba0e84eab3a7a9725/bindings/go/blst.go#L13-L15

// #cgo CFLAGS: -I${SRCDIR}/.. -I${SRCDIR}/../../build -I${SRCDIR}/../../src -D__BLST_CGO__ -fno-builtin-memcpy -fno-builtin-memset
// #cgo amd64 CFLAGS: -D__ADX__ -mno-avx
// #cgo mips64 mips64le ppc64 ppc64le riscv64 s390x CFLAGS: -D__BLST_NO_ASM__

#cgo has no custom flags for 32-bit, I guess because it's not necessary and detected.
Though I'm not sure why specifying the non-amd64 non-aarch64 platforms is necessary either.

Current conclusion

If the custom toolchain does not setup the cross-compilation for zkVM correctly, I doubt #399 will help, the downstream application will be the better place to pass -D__BLST_NO_ASM__.

If the custom toolchain does setup the cross-compilation correctly, then I think BLST will do the right thing™ and actually nothing will be needed on c-kzg-4844 side.

We're continuing investigation.

@jtraglia
Copy link
Member

Thanks for the response. Sounds good, keep us updated. I will close #399.

@jtraglia
Copy link
Member

Hey @mratsim any updates on this?

@mratsim
Copy link
Author

mratsim commented Mar 22, 2024

This is worked on at zkVM-level. Closing.

@mratsim mratsim closed this as completed Mar 22, 2024
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.

2 participants