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

Remove interior mutability hack for chip record generators #87

Closed
iammadab opened this issue Jul 8, 2024 · 2 comments
Closed

Remove interior mutability hack for chip record generators #87

iammadab opened this issue Jul 8, 2024 · 2 comments

Comments

@iammadab
Copy link
Contributor

iammadab commented Jul 8, 2024

After #80 each record-generating chip holds its underlying memory-checking structure in the following way:

pub struct Chip<Ext: ExtensionField> {
    rom_handler: Rc<RefCell<ROMHandler<Ext>>>,
}

The current architecture has a common memory-checking structure from which all chips must read and write.

Rather than having global memory-checking structures for all chips, a better approach will be to have separate memory-checking structures for each chip i.e. on chip instantiation, each chip will create its record-tracker.

After an instruction has finalized all its chip operations, we can combine the records into singular RAM and ROM records.

This removes the interior mutability hack while preserving the chip-first approach.

@hero78119
Copy link
Collaborator

hero78119 commented Jul 10, 2024

Hi, this type definition well address the problem, however I feel like a slightly over-design. I proposed another alternative, aiming for mutability happened in just one place under a struct :)

Besides, to simplify, how about we unify OAMHandler & RamHandler into OAMHandler, and define ram_read/ram_write/ram_read_mixed,ram_write_mixed 4 new function in OAMHandler to aborb RamHandler logic in one place

define new ChipHandler to encaptulate all XXXhandlers internally. So the mutability borrow wont spread everywhere

let mut chip_handler = ChipHandler::new(&mut circuit_builder, challenges);

fn new(challenge) -> Self {
    ...
    ChipHandler {
         rom_handler: ROMHandler::new(challenges.clone))
         oam_handler: OAMHandler::new(challenges))
    }
}

chip operation: take lookup_handler + circuit_builder

For example

RangeChip::range_check_stack_top(&mut lookup_handler, &mut circuit_builder, stack_top_l);

finalize in one call on chip_handler

let (ram_load_id, ram_store_id
, rom_id) = chip_handler.finalize(&mut circuit_builder)

@iammadab
Copy link
Contributor Author

iammadab commented Aug 7, 2024

closed by #94

@iammadab iammadab closed this as completed Aug 7, 2024
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

No branches or pull requests

2 participants