-
Notifications
You must be signed in to change notification settings - Fork 160
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
Fix bug in chiplet bus #1516
Fix bug in chiplet bus #1516
Conversation
OPCODE_JOIN | OPCODE_SPLIT | OPCODE_LOOP | OPCODE_CALL => { | ||
build_control_block_request(main_trace, op_code_felt, alphas, row) | ||
}, | ||
OPCODE_DYN => build_dyn_block_request(main_trace, alphas, row), |
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.
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.
Pushed a commit that fixes this as explained here. Basically DYN
emits a request to hash [ZERO; 8]
using the DYN
domain, which is verified on END
. The block hash table is responsible for ensuring that the correct hash is being executed: the callee hash is pushed on the table on DYN
, and removed on the appropriate END
(when the callee terminates).
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.
It seems like DYN
could not issue any hash request, and also not have an associated END
, since we're only hashing dummy values? So DYN
would still populate the block hash table, but have no other effect on the trace.
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.
It seems like
DYN
could not issue any hash request, and also not have an associatedEND
, since we're only hashing dummy values? SoDYN
would still populate the block hash table, but have no other effect on the trace.
Yeah - I think that's correct.
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.
LGTM! Thank you for fixing this!!
Also pushed a commit fixing how DYN
works, make sure to double check that.
7498350
to
9073401
Compare
9073401
to
eee4477
Compare
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.
Looks good! Thank you!
Describe your changes
Fixes a couple of bugs in the auxiliary column for the chiplet's bus.
Checklist before requesting a review
next
according to naming convention.