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

Extract assignments from witgen inference, step 1 #2452

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

chriseth
Copy link
Member

@chriseth chriseth commented Feb 6, 2025

No description provided.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Benchmarks'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.20.

Benchmark suite Current: 5b05ca2 Previous: 53ad9c4 Ratio
executor-benchmark/keccak 28210735996 ns/iter (± 1327372525) 15309422384 ns/iter (± 73269711) 1.84

This comment was automatically generated by workflow using github-action-benchmark.

@chriseth chriseth force-pushed the extract_assignments_from_witgen_inference branch from ecaf7b2 to dc08751 Compare February 6, 2025 15:31
@chriseth chriseth changed the title Extract assignments from witgen inference Extract assignments from witgen inference, step1 Feb 6, 2025
@chriseth chriseth changed the title Extract assignments from witgen inference, step1 Extract assignments from witgen inference, step 1 Feb 6, 2025
@chriseth chriseth marked this pull request as ready for review February 6, 2025 15:49
row_offset: i32,
) -> ProcessResult<T, Variable> {
self.process_assignments().unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
self.process_assignments().unwrap();
// Make sure any known arguments are stored in the correct machine
// call variable.
self.process_assignments().unwrap();

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah this is actually a hack because it didn't work. I think it's just another manifestation of the bug I want to fix. I will delete this line once witgen_inference has no authority over the assignments any more.

Comment on lines +309 to +313
}) => witgen.process_call(
can_process.clone(),
*identity_id,
&selected_payload.selector,
selected_payload.expressions.len(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why don't we just pass a reference to the BusSend struct? Similarly I think that would be good for the Polynomial case.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we pass a whole BusSend, it gives the impression that it processes the arguments instead of working with the variables.

Copy link
Collaborator

@georgwiese georgwiese left a comment

Choose a reason for hiding this comment

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

Ok!

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.

2 participants