fix: update c.nop encoding and assembly to support HINTs (#1177)#1289
fix: update c.nop encoding and assembly to support HINTs (#1177)#128916srivarshitha wants to merge 7 commits intoriscv:mainfrom
Conversation
Signed-off-by: Srivarshitha M <mvarshitha17874@gmail.com>
ThinkOpenly
left a comment
There was a problem hiding this comment.
Thank you for this contribution. There are a few issues with it for which I've added comments.
A final implementation is perhaps not possible until optional operands are supported.
If you wish to address the addressable comments, please do. This PR will probably need to stay open for some time, though, and then it will need substantial further work. Shall we leave it open until then, or would it be easier to close it and revisit the issue later?
| anyOf: | ||
| - C | ||
| - Zca | ||
| assembly: "c.nop imm" |
There was a problem hiding this comment.
The "assembly" attribute does not include the instruction mnemonic, so this should be changed.
| assembly: "c.nop imm" | |
| assembly: imm |
Also, this is redundant with the "assembly" attribute below. There can be only one.
Also, the "extension" below needs to be under the "definedBy" above, and this was inserted between them.
All that being said, this "imm" operand is optional, and UDB doesn't have a way to specify optional operands at present. Hopefully, this will come with #1527, but that might not be soon.
| module Udb | ||
| class Instruction | ||
| def assembly_fmt(xlen) | ||
| if name == "c.nop" |
There was a problem hiding this comment.
I'd really like to avoid adding special cases for specific instructions in the Ruby code. I suspect this will be easier when operands can be explicitly marked "optional" in the YAML when #1527 is merged.
|
@ThinkOpenly Thanks! I understand we need to wait for #1527 to do this properly without special cases in Ruby. I will remove the Ruby changes and clean up the YAML errors you pointed out. I'll leave this PR open as a draft until the optional operands feature is ready. |
Removed unnecessary definitions for C.NOP instruction. Signed-off-by: Srivarshitha M <mvarshitha17874@gmail.com>
Refactor assembly_fmt and assembly_fmt_args methods in Instruction class to remove special case for 'c.nop'. Signed-off-by: Srivarshitha M <mvarshitha17874@gmail.com>
|
|
||
| module Udb | ||
| class Instruction | ||
| class Instruction |
There was a problem hiding this comment.
I think these changes are inadvertent, and need to be reverted?
Description
This PR resolves the encoding divergence for c.nop between the unified database and riscv-opcodes, as reported in issue #1177.
Changes
YAML (
c.nop.yaml):Updated the match pattern from the fully constrained value
0x0001to000-00000-----01, using wildcards for the immediate fieldThis change enables the database to correctly recognize c.addi x0, x0, imm (where imm ≠ 0) as c.nop HINT variants
Generator (
template_helpers.rb):Enhanced assembly_fmt and assembly_fmt_args to support dynamic formatting based on the immediate value:
When imm = 0: displays as c.nop
When imm ≠ 0 (HINT variant): displays as c.nop
Verification
Fixes #1177