fix(balanced_decomposition): reconstruct modulus from limbs#1028
fix(balanced_decomposition): reconstruct modulus from limbs#1028VolodymyrBg wants to merge 5 commits intoingonyama-zk:mainfrom
Conversation
VolodymyrBg
commented
Aug 13, 2025
VolodymyrBg
commented
- Replace pointer-punning of modulus ((const int64_t)&q_storage) with portable 64-bit reconstruction from 32-bit limbs.
- Eliminates strict-aliasing UB and endianness dependence; keeps behavior unchanged on little-endian targets.
- Add include for std::log2/std::ceil usage.
- Update assert message to reference 64-bit arithmetic expectations.
|
All contributors have signed the CLA ✍️ ✅ |
| constexpr auto q_storage = T::get_modulus(); | ||
| const int64_t q = *(const int64_t*)&q_storage; | ||
| ICICLE_ASSERT(q > 0) << "Expecting at least one slack bit to use int64 arithmetic"; | ||
| const uint64_t q_u = (static_cast<uint64_t>(q_storage.limbs[0])) | |
There was a problem hiding this comment.
(1) I think you assume limbs are 32b so please add a static assert
(2) now that it is uint64, the ICICLE_ASSERT(q_u > 0) is redundant since uint is always >0. The original intent was to make sure we have a slack bit.
(3) If I recall correctly similar code also exists in cpu_balanced_decomposition code, please fix it as well.
yshekel
left a comment
yshekel
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
Did you have issues with this code? in which machine?
I left a comment, please fix and I will approve the PR.
|
I have hereby read the ICICLE CLA and agree to its terms |
Hi, fixed |
Formatting is fixed, but why CLA is not signed i don't know |
|
recheck |
how? no link to sign |
Apologies, thats a trigger for the CI to auto recheck the signing. There is a bug in the CI script that we will fix to get this to work correctly |
4 participants
