-
Notifications
You must be signed in to change notification settings - Fork 118
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
Feat/vlad/signed msm #537
Feat/vlad/signed msm #537
Conversation
@@ -103,6 +106,7 @@ namespace msm { | |||
false, // are_results_on_device | |||
false, // is_big_triangle | |||
false, // is_async | |||
false, // is_signed |
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.
If this allows for using less memory AND is faster, why don't we enable is as a default?
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.
he mentioned that it only works for with the big-triangle accumulation that is slower compared to the other accumulation algorithm.
@vladfdp can you say how does big-triangle + signed compared to the other accumulation without signed optimization?
@@ -73,6 +77,7 @@ func GetDefaultMSMConfig() MSMConfig { | |||
false, // areResultsOnDevice | |||
false, // IsBigTriangle | |||
false, // IsAsync | |||
false, //IsSigned |
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.
missing space
Describe the changes
This PR adds an option to run msm using signed digits.
Currently only works with big triangle
I set the default is_signed to false because big triangle is false by default.
I made the split scalars kernel into two different ones for readibility but I can make it a single one if necessary
Linked Issues
Resolves #