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

Thumb: Implement MultipleLoadStore #189

Merged
merged 1 commit into from
May 16, 2023
Merged

Thumb: Implement MultipleLoadStore #189

merged 1 commit into from
May 16, 2023

Conversation

guerinoni
Copy link
Collaborator

@guerinoni guerinoni commented May 15, 2023

This implements only the part clementine needs to pass the step. We should improve testing and splitting better files like module arm and module thumb

@guerinoni guerinoni self-assigned this May 15, 2023
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 96.15% and project coverage change: +0.40 🎉

Comparison is base (fbda12d) 57.13% compared to head (2ccc3a3) 57.54%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #189      +/-   ##
==========================================
+ Coverage   57.13%   57.54%   +0.40%     
==========================================
  Files          37       37              
  Lines        2753     2777      +24     
==========================================
+ Hits         1573     1598      +25     
+ Misses       1180     1179       -1     
Impacted Files Coverage Δ
emu/src/cpu/instruction.rs 82.26% <91.66%> (+0.87%) ⬆️
emu/src/cpu/arm7tdmi.rs 83.39% <100.00%> (+0.39%) ⬆️
emu/src/cpu/opcode.rs 88.73% <100.00%> (+0.16%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

let base_address = self.registers.register_at(base_register as usize);
for r in 0..=7 {
if register_list.get_bit(r) {
let address = base_address + (r as u32 * 4);
Copy link
Collaborator

@AlessioC31 AlessioC31 May 16, 2023

Choose a reason for hiding this comment

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

I'm not sure this is correct, imagine a register list like this: 0b00000010 and R0 = 0x100. We should then read at 0x100 and write in R1. But with this logic we read in 0x104, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right! Fixed the behavior, only increment if bit is true :)

let mut registers = String::new();
for i in 0..=7 {
if register_list.get_bit(i) {
registers.push_str(&format!("R{}, ", i));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Like this we would have {R0, R1, }, right? (I mean at the end we would have an additional comma).

Also, to be consistent with the datasheet we should use - to separate registers :P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I know... Right now I took a shortcut :D we can leave as good first issue :) (another instruction has this problem) :D

@guerinoni guerinoni force-pushed the MultipleLoadStore branch from 2ccc3a3 to 318cedd Compare May 16, 2023 08:16
emu/src/cpu/arm7tdmi.rs Outdated Show resolved Hide resolved
@guerinoni guerinoni force-pushed the MultipleLoadStore branch from 318cedd to 5d40285 Compare May 16, 2023 08:53
emu/src/cpu/arm7tdmi.rs Outdated Show resolved Hide resolved
Signed-off-by: Federico Guerinoni <guerinoni.federico@gmail.com>
@AlessioC31 AlessioC31 added this pull request to the merge queue May 16, 2023
Merged via the queue into main with commit 2647c9e May 16, 2023
@guerinoni guerinoni deleted the MultipleLoadStore branch May 27, 2023 12:31
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.

3 participants