-
Notifications
You must be signed in to change notification settings - Fork 155
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
lowram implementation #91
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some small suggestions.
Co-authored-by: Matthias J. Kannwischer <[email protected]>
Hi Armin, Thanks a lot for submitting the PR. I have now had a closer look at it. It would be great if you could make a pass over the code and clean it up a bit more. Some of the things I have noticed are inconsistencies in types (unsigned char instead of uint8_t, size_t instead of unsigned int), missing const qualifiers, and missing namespace defines. Also it would be good to use the same coding style as in the rest of the Dilithium code (no spaces after for, if and so on, { on the same line, variable declarations only at function beginnings). Regarding the approach of retaining as much of the ref implementation as possible: In principle I like the idea. But it leads to code duplication that might be harder to maintain than replacing the ref files such as packing.c and poly.c with variants that implement the new functions in the expected places. So thinking about it I would prefer that. Cheers, |
Hi Gregor, thanks for reviewing this!
I'm not sure I understand this. The reason we went for this approach was to reduce code duplication. All functions specific to this implementations are moved to |
Hi Matthias, Your explanation makes a lot of sense. The reason why I think I prefer independent copies of the ref source files where the lowram function variants are implemented is the following. Let's assume future compilers turn the rounding implementation into variable time code and we need to change it. Then, we know we also have to look into the rounding.c files of the avx2 and lowram implementations. On the other hand when rounding.c in lowram/ is a symlink but the same code is duplicated in lowram.c, we have to remember this and I think there is a risk that we forget. And yes, the dead code you mentioned might also a bit annoying if someone is only interested in the lowram implementation and would like to have a clean stand-alone implementation. On the other hand you're of course right that copying the sources files leads to much more code duplication than your approach. But hopefully the code doesn't actually have to be changed much in the future. So in summary I guess I weigh clean stand-alone implementations higher than less code duplication. Cheers, |
I suggest precomputing the constant in Barrett in mod 3329. The constant is 20159. |
I second with Gregor that standalone implementation is preferable. The stack-optimized implementation targets different users. |
This PR adds a third implementation variant called
lowram
. It focuses on using very little memory at a performance tradeoff. The implementation is written in C and based on the code in pqm4, which was added in this PR.The original ideas for this implementation are taken from the paper
and parts of the implementation in this PR are written by @mkannwischer.
I tried to retain as many files as possible from the
ref
implementation by moving most memory-optimization related code tolowram.c
. Further,smallntt_3329.c
andsmallpoly.c
contain functions to operate on polynomials with 16-bit coefficients.api.h
,config.h
,Makefile
, andsign.c
are the only files that should be different fromref
-- everything else is just symlinks.If there's anything that could be done to improve the quality of the implementation, feel free to let me know!
Cheers,
Amin