asm: Improve documentation and code quality for RISC-V instructions #341
Conversation
- Remove redundant unimplemented!() calls on non-RISC-V targets since
functions are already properly gated with cfg attributes
- Add comprehensive safety documentation explaining when and why
instructions are unsafe (ebreak, ecall)
- Enhance behavior descriptions with practical use cases and performance
considerations for fence operations
- Add preserves_flags option to instructions that don't modify flags
(nop, wfi, ebreak, ecall)
- Fix sfence_vma() assembly template to use idiomatic {} syntax
- Strengthen delay() function warnings about timing accuracy limitations
and recommend proper timer peripherals for precise delays
- Add examples for sfence_vma() showing ASID and address targeting
- Improve multiprocessor considerations documentation for fence_i
- Standardize documentation format with consistent Safety, Behavior,
and Use Cases sections
These changes maintain full backward compatibility while significantly
improving developer experience and preventing common usage mistakes.
There was a problem hiding this comment.
Hi! I noticed that the git history is quite dirty (9 commits), and you included a few unnecessary/wrong indentation/space changes.
Please, start from a fresh, clean fork of this repo, include only the changes in the documentation and preserves_flags, and open a new PR with only the changes needed. Otherwise, it is very difficult to review, as there are changes everywhere and I cannot easily keep track of what has actually changed and what has not.
| @@ -0,0 +1,5 @@ | |||
| ## [Unreleased] | |||
There was a problem hiding this comment.
Please, remove this file completely
| @@ -1,282 +1,4 @@ | |||
| # Change Log | |||
There was a problem hiding this comment.
It looks like your branch is not up to date with the master branch. Please, rebase from master to get the latest changes. Also, note that the idea is not removing anything from the changelog, but progressively adding and documenting changes across versions. The newest changes (for example, the ones on your PR) go on top of the file under the [Ureleased] section. Please, take a look at http://keepachangelog.com/ for more information about how we record changes in the repo.
riscv/src/register.rs
Outdated
| // Debug Mode Registers | ||
| pub mod dcsr; |
There was a problem hiding this comment.
Do not add registers in this PR. It is better to leave these changes to #342
|
@romancardenas Thanks for putting time to my PR! I have cleanly rebased this entire branch with fixed indentations, and the CHANGELOG.md is completely fixed, aswell. |
KushalMeghani1644
force-pushed
the
improvement
branch
from
6ddabd1 to
5944836
Compare
Improve documentation and code quality for RISC-V assembly instructions
Summary
Changes Made:
Documentation Improvements:
Enhanced safety documentation: Added comprehensive safety sections explaining when and why instructions like ebreak and ecall are unsafe, including specific warnings about exception handling and stack pointer management.
Expanded behavioral descriptions: Provided detailed explanations of instruction behavior, including privilege mode effects for ecall and power management implications for wfi.
Added practical use cases: Documented appropriate use cases for fence operations, including multiprocessor considerations and performance implications.
Improved function parameter documentation: Enhanced documentation for sfence_vma() with clear parameter descriptions and usage examples.
Code Quality Enhancements:
Removed redundant code: Eliminated unnecessary unimplemented!() calls on non-RISC-V targets since functions are already properly gated with cfg attributes.
Added missing assembly options: Included preserves_flags option for instructions that do not modify processor flags (nop, wfi, ebreak, ecall).
Fixed assembly template syntax: Updated sfence_vma() to use idiomatic {} placeholder syntax instead of explicit numbering.
Improved code organization: Restructured the delay() function implementation for better readability.
Enhanced Warning Documentation:
Strengthened timing accuracy warnings: Added comprehensive warnings about the delay() function's limitations, including interrupt interference and architecture dependencies, with strong recommendations to use proper timer peripherals for precise timing requirements.
Added performance guidance: Documented the performance implications of fence operations and recommended using specific sfence_vma() over sfence_vma_all() when possible.
Testing:
Compatibility