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

AR: Clarify message register uses. #867

Merged
merged 1 commit into from
Aug 22, 2023
Merged

AR: Clarify message register uses. #867

merged 1 commit into from
Aug 22, 2023

Conversation

timsifive
Copy link
Contributor

aampostincrement implies the implementation does not use simple MRs.

Copy link
Collaborator

@pdonahue-ventana pdonahue-ventana left a comment

Choose a reason for hiding this comment

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

This seems contradictory. The first change says that data0-data11 are MRs. The second change says that in order for the DM to increment arg1, the data registers can't be MRs.

Maybe the idea is that data0-data11 may be implemented as MRs or they may be implemented as normal registers. Then the second thing could say that they must be implemented as normal registers and not MRs if you want incrementing to work.

@timsifive
Copy link
Contributor Author

Yes, I think that would resolve the issue.

Copy link
Collaborator

@pdonahue-ventana pdonahue-ventana left a comment

Choose a reason for hiding this comment

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

You could also say that there are two ways to implement message registers:

  • writes to PortA update ValueA and leave ValueB UNSPECIFIED (the MR way that you've described)
  • writes to PortA update both ValueA and ValueB

The latter is really just a normal register. The above description provides an alternative to having dataX be MRs in some implementations and not MRs in others. Instead, they'd always be MRs but some implementations would have deterministic behavior for the other value.

Having said that, it's probably simpler for the reader to just see "normal register" than having to mentally map this abstract two port description back to this fundamental concept (a straightforward register) that they're already familiar with. I just mention it since I thought of it and wondered if you agree that your way is simpler.

debug_module.tex Outdated Show resolved Hide resolved
Copy link
Collaborator

@pdonahue-ventana pdonahue-ventana left a comment

Choose a reason for hiding this comment

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

This is fine. Do we run this by ARC as a draft proposal or do we merge this into the spec and then have them give feedback at that stage? The latter seems more cumbersome (due to more back and forth) but that might be what they're expecting.

@timsifive
Copy link
Contributor Author

I'm inclined to leave it as is and not confuse the reader by talking about how the MR description relates to a normal register.

data* registers may be MRs, instead of are definitely MRs. This lets us
say that if aampostincrement is implemented then they must not be MRs.

Also rewrote the definition to be more clear.
@timsifive
Copy link
Contributor Author

This is fine. Do we run this by ARC as a draft proposal or do we merge this into the spec and then have them give feedback at that stage? The latter seems more cumbersome (due to more back and forth) but that might be what they're expecting.

I think it makes the spec better, so let's merge it. I'm planning to merge fixes to address all the preliminary feedback, and then the ball is all the way back in their court.

@timsifive timsifive merged commit c13d9bd into master Aug 22, 2023
1 check passed
@timsifive timsifive deleted the message_reg branch August 22, 2023 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants