Skip to content

rv_zimop/rv_zcmop - rename mneumonics to use all lowercase & simplify operand names - #417

Open
robinali-codasip wants to merge 2 commits into
riscv:masterfrom
robinali-codasip:rv_zimop_update
Open

rv_zimop/rv_zcmop - rename mneumonics to use all lowercase & simplify operand names#417
robinali-codasip wants to merge 2 commits into
riscv:masterfrom
robinali-codasip:rv_zimop_update

Conversation

@robinali-codasip

@robinali-codasip robinali-codasip commented Apr 9, 2026

Copy link
Copy Markdown
Contributor
  • mop.r{r}.N -> mop.r{r}.n to keep consistency
  • mop_r{r}_t -> mop_r{r}
    Same for c.mop.n

Issue #416 - Change from pseudo is not implemented here so favour for that would be appreciated here

* mop.r{r}.N -> mop.r{r}.n to keep consistency
* mop_r{r}_t -> mop_r{r}
@robinali-codasip robinali-codasip changed the title rv_zimop - rename mneumonics to use all lowercase & simplify field names rv_zimop - rename mneumonics to use all lowercase & simplify operand names Apr 9, 2026
@codecov

codecov Bot commented Apr 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.09%. Comparing base (4644ba3) to head (75aa877).
⚠️ Report is 16 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #417   +/-   ##
=======================================
  Coverage   97.08%   97.09%           
=======================================
  Files          14       14           
  Lines         926      929    +3     
=======================================
+ Hits          899      902    +3     
  Misses         27       27           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@robinali-codasip
robinali-codasip force-pushed the rv_zimop_update branch 2 times, most recently from 30b1ac6 to 388b857 Compare April 9, 2026 14:04
@robinali-codasip robinali-codasip changed the title rv_zimop - rename mneumonics to use all lowercase & simplify operand names rv_zimop/rv_zcmop - rename mneumonics to use all lowercase & simplify operand names Apr 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a backwards-incompatible change for downstream users, and it isn't clear to me that it's necessary. It is true that the N is unusual, but there is an arguable justification for that: there is no such instruction mnemonic as c.mop.N, as the N is a placeholder for an integer in the mnemonic.

@robinali-codasip

Copy link
Copy Markdown
Contributor Author

Andrew Waterman (@aswaterman) I agree that it's not ideal for backwards compatibility, but I am not convinced myself that improvements can be ignored in favour of maintaining it. Would you be more likely to accept a change if I also removed the pseudo definitions and make each instruction its own entry? i.e c.mop.0, c.mop.1 etc. (removing c.mop.N). I believe that it makes it better for future extensions that use the mop space.

@aswaterman

Copy link
Copy Markdown
Member

I guess I'm not sure I agree it's an improvement. What's the problem, exactly, aside from an apparent inconsistency? (And as I wrote above, I still think the inconsistency is justified since N is a placeholder for an integer, rather than part of an official mnemonic.)

Getting rid of c.mop.N will be more disruptive than renaming it to c.mop.n, FWIW.

@robinali-codasip

Copy link
Copy Markdown
Contributor Author

I understand that getting rid of c.mop.N altogether will be more disruptive, it's what I think is really the issue here. It obfuscates what the instructions and mneumonics are, for example adding a new instruction that uses the mop space will need to create an identical pseudo to an existing entry here instead of a new pseudo which maps one-to-one. I think it's a bit like having one entry for addsub and then add and sub pseudos. The idea behind suggesting the new change instead is that it's a full solution rather than a half solution currently in the current PR. I do now agree that c.mop.N -> c.mop.n is probably not worth the disruption. But I do not right now for the full change.

@aswaterman

Copy link
Copy Markdown
Member

OK, well, I'm on record not supporting that change.

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