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

Implement ll/lld/sc/scd #185

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Implement ll/lld/sc/scd #185

wants to merge 1 commit into from

Conversation

clbr
Copy link
Contributor

@clbr clbr commented Dec 26, 2020

No description provided.


// We need to mark the line dirty if it's write.
// Metadata & 0x2 == dirty, and metadata 0x1 == valid. Map to those.
if (request->type == VR4300_BUS_REQUEST_READ_CONDITIONAL)
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be checking for VR4300_BUS_REQUEST_STORE_CONDITIONAL?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this block may be dedup'd with the original store/load path, replacing this if statement with:
line->metadata |= (exdc_latch->request.type & 0x3);

and carefully picking constant vals for VR4300_BUS_REQUEST_READ_CONDITIONAL and VR4300_BUS_REQUEST_WRITE_CONDITIONAL.

return 0; // fail is not an exception
} else {
// Successful store, set the reg to 1
vr4300->regs[exdc_latch->request.sc_reg] = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be a pipelined write? Otherwise, the result may not be correctly forwarded:
https://github.com/n64dev/cen64/blob/master/vr4300/pipeline.c#L173

dcwb_latch->dest = exdc_latch->request.sc_reg;
dcwb_latch->result = 1

@tj90241
Copy link
Collaborator

tj90241 commented Dec 26, 2020

Read conditional seems like it would create LDIs as well:
https://github.com/n64dev/cen64/blob/master/vr4300/pipeline.c#L164

vr4300->regs[VR4300_CP0_REGISTER_LLADDR] != paddr >> 4) {
// Fail the store, set the reg to 0, write nothing to memory
exdc_latch->request.type = VR4300_BUS_REQUEST_NONE;
vr4300->regs[exdc_latch->request.sc_reg] = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Must be written by WB for forwarding:

dcwb_latch->dest = exdc_latch->request.sc_reg;
dcwb_latch->result = 0;

@clbr
Copy link
Contributor Author

clbr commented Dec 27, 2020

Thanks for the quick review.

shouldn't this be checking for VR4300_BUS_REQUEST_STORE_CONDITIONAL?
That's in the else case, just like in the original code I copied from? (can't reply inline, github is bork)

@tj90241
Copy link
Collaborator

tj90241 commented Dec 27, 2020

ahh okay, yes, you're correct - I had thought it was reversed.
FWIW, you only need to set the dirty bit in the case of stores. Valid is already set, or the cache probe would have missed. I had used the mask like that to avoid a branch by playing with the constants.

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