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

feat: widemul128 hint #50

Merged
merged 11 commits into from
Oct 12, 2023
Merged

Conversation

greged93
Copy link
Contributor

This PR adds the widemul128 hint to the hint list. Tests are provided.
Resolves #37.

Copy link
Contributor

@rodrigo-pino rodrigo-pino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks for your contribution, some small changes and it is ready for approval!

pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint_test.go Outdated Show resolved Hide resolved
@rodrigo-pino
Copy link
Contributor

@greged93 I believe this library I mentioned on #67 (comment) can be of use. I think it will help make the operations faster leaving only the cost of converting to montgomery form at the end.

@greged93
Copy link
Contributor Author

greged93 commented Sep 29, 2023

Updated @rodrigo-pino!

@omerfirmak
Copy link
Contributor

pkg/hintrunner/hint.go:106:7: leaking param: hint
pkg/hintrunner/hint.go:106:32: leaking param: vm
pkg/hintrunner/hint.go:109:2: moved to heap: lhs
pkg/hintrunner/hint.go:114:2: moved to heap: rhs
pkg/hintrunner/hint.go:152:2: moved to heap: mvLow
pkg/hintrunner/hint.go:162:2: moved to heap: mvHigh
pkg/hintrunner/hint.go:144:29: moved to heap: uint256.words
pkg/hintrunner/hint.go:146:31: moved to heap: uint256.words
pkg/hintrunner/hint.go:107:28: &fp.Element{...} does not escape
pkg/hintrunner/hint.go:111:20: ... argument does not escape
pkg/hintrunner/hint.go:116:20: ... argument does not escape
pkg/hintrunner/hint.go:124:20: ... argument does not escape
pkg/hintrunner/hint.go:132:20: ... argument does not escape
pkg/hintrunner/hint.go:135:23: &uint256.Int{} does not escape
pkg/hintrunner/hint.go:135:76: []big.Word{...} escapes to heap
pkg/hintrunner/hint.go:135:76: &big.Int{...} does not escape
pkg/hintrunner/hint.go:135:50: &uint256.Int{} does not escape
pkg/hintrunner/hint.go:135:50: "overflow" escapes to heap
pkg/hintrunner/hint.go:135:128: []big.Word{...} escapes to heap
pkg/hintrunner/hint.go:135:128: &big.Int{...} does not escape
pkg/hintrunner/hint.go:135:102: &uint256.Int{} does not escape
pkg/hintrunner/hint.go:135:102: "overflow" escapes to heap
pkg/hintrunner/hint.go:138:23: &uint256.Int{} does not escape
pkg/hintrunner/hint.go:139:24: &uint256.Int{} does not escape
pkg/hintrunner/hint.go:143:13: &fp.Element{} does not escape
pkg/hintrunner/hint.go:144:29: new(big.Int) does not escape
pkg/hintrunner/hint.go:145:14: &fp.Element{} does not escape
pkg/hintrunner/hint.go:146:31: new(big.Int) does not escape
pkg/hintrunner/hint.go:150:20: ... argument does not escape
pkg/hintrunner/hint.go:155:20: ... argument does not escape
pkg/hintrunner/hint.go:160:20: ... argument does not escape
pkg/hintrunner/hint.go:165:20: ... argument does not escape

There are multiple GC allocations here. func (hint WideMul128) Execute could be allocation free AFAICS.

@rodrigo-pino
Copy link
Contributor

There are multiple GC allocations here. func (hint WideMul128) Execute could be allocation free AFAICS.

I've been analizing this. lhs and rhs escapes because their FieldElement() and MemoryAddress() methods return pointers instead of the value. I made a local change that fixed it. So that is smth that we should fix on our side. Let me look at the other heap allocations

@rodrigo-pino
Copy link
Contributor

rodrigo-pino commented Oct 6, 2023

mvHigh and mvLow I am not sure. They shouldn't imo. But that part is on our side as well I because it is not used in the code, so I think it is good to merge once the comment is resolved and fix that later since changes required modifications on other parts of the code. @omerfirmak tell me what you think

pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
@omerfirmak
Copy link
Contributor

lhs and rhs escapes because their FieldElement() and MemoryAddress() methods return pointers instead of the value.

That doesnt seem right to me, we have this pattern everywhere and it doesnt cause heap allocations elsewhere.

I am more suspicious of the interfaces that are in operand.go.

pkg/hintrunner/hint.go Outdated Show resolved Hide resolved
@rodrigo-pino rodrigo-pino merged commit a0ec8a6 into NethermindEth:main Oct 12, 2023
4 checks passed
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 this pull request may close these issues.

Feat: Implement WideMul128 Hint
3 participants