-
Notifications
You must be signed in to change notification settings - Fork 50
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
EltwiseReduceMod fails on EltwiseReduceModAVX512 operation with a modulus that is between 51 and 52 bits. #121
Comments
Hello @jcalafato1 I have reproduced this issue with a modulus size of 52 bits but not 51 bits. Do you have a 51 bits case that you can share? I think this is a limitation of the AVX512-IFMA (52 bits) algorithm on big inputs. |
Hey @joserochh thanks for getting back to me, is a fix available or is it just a limitation of the library? I have a workaround in my projects to prevent 52 bit moduli from being generated right now, but I'd rather not have the restriction. I also misspoke, when i said between 51 and 52 bits i meant numbers between 2^51 and 2^52, which are 52 bit numbers. |
There will be a fix so you don't need to avoid 52 bit moduli. In the worst case, the library will have to use AVX512-DQ intrinsic functions instead of AVX512-IFMA for this scenario. When input values are not that big, say only up to 4 times modulus value (input_mod_factor = 4), then reduction works fine with 52 bit moduli on IFMA as the algorithm is way simpler. |
Sounds good, thanks! I'll watch for a new version. |
Fix is on dev branch. |
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [com_intel_hexl](https://togithub.com/intel/hexl) | http_archive | patch | `v1.2.4` -> `v1.2.5` | --- ### Release Notes <details> <summary>intel/hexl (com_intel_hexl)</summary> ### [`v1.2.5`](https://togithub.com/intel/hexl/releases/tag/v1.2.5) [Compare Source](https://togithub.com/intel/hexl/compare/v1.2.4...v1.2.5) - Adds experimental FFT-like ([https://github.com/intel/hexl/pull/104](https://togithub.com/intel/hexl/pull/104)) - Adds big moduli tests for IFMA ([https://github.com/intel/hexl/pull/123](https://togithub.com/intel/hexl/pull/123)) - Fixes HEXL's example build ([https://github.com/intel/hexl/pull/114](https://togithub.com/intel/hexl/pull/114)) - Fixes IFMA/DQ logic error ([https://github.com/intel/hexl/pull/118](https://togithub.com/intel/hexl/pull/118)) - Fixes pre-built CpuFeatures Error ([https://github.com/intel/hexl/pull/120](https://togithub.com/intel/hexl/pull/120)) - Fixes 52-bit modulus issue [https://github.com/intel/hexl/issues/121](https://togithub.com/intel/hexl/issues/121) ([https://github.com/intel/hexl/pull/123](https://togithub.com/intel/hexl/pull/123)) - Updates to documentation Co-Authored-by: [@​joserochh](https://togithub.com/joserochh) Co-Authored-by: [@​hamishun](https://togithub.com/hamishun) Co-Authored-by: [@​jlhcrawford](https://togithub.com/jlhcrawford) Full Changelog: intel/hexl@v1.2.4...v1.2.5 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/secretflow/spu). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMjcuMCIsInVwZGF0ZWRJblZlciI6IjM3LjEyNy4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
The library does not reduce properly for any modulus between 51 and 52 bits. See this test:
TEST(ReduceFailure, ReduceFailure) {
std::vector<uint64_t> tst = {
41834706795195925,
4670328046076965,
17760383643480343,
49435670237278413,
24379914680392825,
33362919182756282,
14501318335168678,
31183658415687847
};
auto mod = 4440955546175441;
auto chk = tst;
intel::hexl::EltwiseReduceMod(chk.data(), tst.data(), tst.size(),
mod,
mod, 1);
auto exp = tst;
for (auto & elem : exp) {
elem %= mod;
}
ASSERT_EQ(exp, chk);
}
This is just an example. I have tried several different moduli of the same bit width and several different inputs. The reduction function does not match the modulus operation.
The text was updated successfully, but these errors were encountered: