Skip to content

Mips fix instruction memory view#104

Merged
Spooky-Firefox merged 25 commits intoperlindgren:masterfrom
jakobkieri:mips_fix_instruction_memory_view
Sep 5, 2025
Merged

Mips fix instruction memory view#104
Spooky-Firefox merged 25 commits intoperlindgren:masterfrom
jakobkieri:mips_fix_instruction_memory_view

Conversation

@jakobkieri
Copy link
Copy Markdown
Contributor

@jakobkieri jakobkieri commented Jul 17, 2025

Fixed:

  • PC_IM stays correct, even when unclocking

Added:

  • Dynamic symbols representing the other three different pipeline stages: PC_DE, PC_EX, PC_DM
  • Instruction memory behaviour when unclocking
  • Instruction memory behaviour when reseting

Comment thread mips-lib/src/components/mips_im.rs Outdated
pub fn update_dynamic_symbols(&self, new_pc: u32) {
let pc_history = self.pc_history.borrow();
let mut new_dynamic_symbols = self.dynamic_symbols.borrow_mut().clone();
if new_dynamic_symbols.contains_key("PC_IM") {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There seams to be a lot of nested if statments, is there a better way to write this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes, it is now improved. Thank you

Comment thread mips-lib/src/components/mips_im.rs Outdated
}
pub fn update_dynamic_symbols(&self, new_pc: u32) {
let pc_history = self.pc_history.borrow();
let mut new_dynamic_symbols = self.dynamic_symbols.borrow_mut().clone();
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why borrow_mut when we clone?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is it even necceasy to do it this way? can't we modify it inplace?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right, I have now successfully fixed it (commit: 35e8e7e)

Comment thread mips-lib/src/components/mips_im.rs Outdated
pub mem_view: RefCell<MemViewWindow>,

#[serde(skip)]
pub pc_history: RefCell<Vec<u32>>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

i understand why this is necessary, since we don't get a data when unclocking. But does there exist a better way?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes and I have replaced pc_history with pc_dm_history. I have also atleast made the situation better by only relying on pc_dm_history when it comes to the dynamic symbol PC_DM

Copy link
Copy Markdown
Collaborator

@Spooky-Firefox Spooky-Firefox left a comment

Choose a reason for hiding this comment

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

Added some comments but they are nitpicks and don't need to be resolved, except not clearing pc history vec on reset.

another thing that might be good for learning is to rebase the branch to squash all "cleaning" "removed row" commits into one.

otherwise LGTM

Comment thread mips-lib/src/components/mips_im.rs
Comment thread mips-lib/src/components/mips_im.rs
Comment thread mips-lib/src/components/mips_im.rs
jakobkieri and others added 2 commits September 3, 2025 16:27
removed row

removed row

fix comment

cleaning
@jakobkieri jakobkieri force-pushed the mips_fix_instruction_memory_view branch from 889be68 to 464bf35 Compare September 3, 2025 14:29
Copy link
Copy Markdown
Collaborator

@Spooky-Firefox Spooky-Firefox left a comment

Choose a reason for hiding this comment

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

lgtm

@Spooky-Firefox Spooky-Firefox merged commit 8963b7d into perlindgren:master Sep 5, 2025
4 checks passed
@jakobkieri jakobkieri deleted the mips_fix_instruction_memory_view branch September 10, 2025 22:33
@jakobkieri jakobkieri restored the mips_fix_instruction_memory_view branch September 12, 2025 20:52
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