Various fixes for benchmarks #62
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tim-hoffman
force-pushed
the
th/poseidon-fix-extract
branch
from
3f79ff1 to
a169a98
Compare
tim-hoffman
force-pushed
the
th/poseidon-fix-extract
branch
from
a169a98 to
4b9feca
Compare
…versions of the subcomponent
tim-hoffman
force-pushed
the
th/benchmark-fixes
branch
from
1fd2d91 to
c1ee0be
Compare
iangneal
requested changes
Oct 30, 2023
| // "loop.body" function, this branch is dead code, thus no parameter was added | ||
| // to the function to reference the destination of this StoreBucket and the | ||
| // location information was not updated so there is an invalid parameter reference | ||
| // that causes 'functions.rs::get_arg_ptr' to crash, but it's in dead code. |
There was a problem hiding this comment.
Maybe update the comment here since we're not crashing at get_arg_ptr anymore.
There was a problem hiding this comment.
Oof, I added the checks but didn't even put the correct command at the start to use the checks.
| //CHECK-NOT: define void @..generated..loop.body.[[[0-9]+]]( | ||
| //CHECK: define void @..generated..loop.body.[[[0-9]+]].T( | ||
| //CHECK-NOT: define void @..generated..loop.body.[[[0-9]+]]( | ||
| //CHECK: define void @..generated..loop.body.[[[0-9]+]].F.T( |
There was a problem hiding this comment.
I'm a little confused why we end up generating:
@..generated..loop.body.[[[0-9]+]].T(...)
@..generated..loop.body.[[[0-9]+]].F(...)
@..generated..loop.body.[[[0-9]+]].F(...)
for the first 3 iterations of the unrolled loop, but
@..generated..loop.body.[[[0-9]+]].F.T(...)
@..generated..loop.body.[[[0-9]+]].F.T(...)
@..generated..loop.body.[[[0-9]+]].F.T(...)
for the last three iterations. Shouldn't we be able to generate
@..generated..loop.body.[[[0-9]+]].F(...)
@..generated..loop.body.[[[0-9]+]].F(...)
@..generated..loop.body.[[[0-9]+]].F(...)
For the last three iterations?
There also seems to be a bug here, because if I change OR to:
template OR() {
signal input a;
a === 0;
}I find that OR_0_run is only called by @..generated..loop.body.[[[0-9]+]].F.T and not @..generated..loop.body.[[[0-9]+]].F or @..generated..loop.body.[[[0-9]+]].F.T, so the first three iterations end up omitting the body of OR_0_Run.
There was a problem hiding this comment.
Yeah, it really should have 6 calls to the run function. Part of what's going on here is addressed in some of the other PRs so I'll make a JIRA ticket to re-check this after all of this mess is merged in.
There was a problem hiding this comment.
circuit_passes/src/bucket_interpreter/env/extracted_func_env.rs
Outdated
Show resolved
Hide resolved
circuit_passes/src/bucket_interpreter/env/extracted_func_env.rs
Outdated
Show resolved
Hide resolved
iangneal
approved these changes
Oct 30, 2023
tim-hoffman
added a commit
that referenced
this pull request
Oct 31, 2023
* skip body extraction when the loop has 0 iterations * Modify interpreter to distinguish between circom functions and loop body functions * [VAN-676] duplicate extracted functions when flattening branch buckets * [VAN-670] generate and flatten subcmp counter/run * impl Debug for Env to be more friendly, expand on test case comment * remove debug println * fix: conditions not flattening in non-generated methods * [VAN-582] properly convert mapped to indexed when there are multiple versions of the subcomponent * fix XFAIL test outputs so they pass * add test cases * handle CallBucket return case in mapped_to_indexed * fix body_extractor assertion failure * fix segfault from improper GEP * use BucketId as key to speed up SimplificationPass * fix function name to reflect actual usage * Remove functions that cannot be reached from any template Co-authored-by: Daniel Dominguez <[email protected]>
tim-hoffman
added a commit
that referenced
this pull request
Nov 8, 2023
* skip body extraction when the loop has 0 iterations * Modify interpreter to distinguish between circom functions and loop body functions * [VAN-676] duplicate extracted functions when flattening branch buckets * [VAN-670] generate and flatten subcmp counter/run * impl Debug for Env to be more friendly, expand on test case comment * remove debug println * fix: conditions not flattening in non-generated methods * [VAN-582] properly convert mapped to indexed when there are multiple versions of the subcomponent * fix XFAIL test outputs so they pass * add test cases * handle CallBucket return case in mapped_to_indexed * fix body_extractor assertion failure * fix segfault from improper GEP * use BucketId as key to speed up SimplificationPass * fix function name to reflect actual usage * Remove functions that cannot be reached from any template Co-authored-by: Daniel Dominguez <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.