Add explicit PC metadata for instructions (#1825)#1827
Add explicit PC metadata for instructions (#1825)#1827amkr6207 wants to merge 2 commits intoriscv:mainfrom
Conversation
Signed-off-by: Aman <amkr6207@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1827 +/- ##
==========================================
- Coverage 72.21% 72.19% -0.03%
==========================================
Files 52 52
Lines 27740 27746 +6
Branches 6012 6002 -10
==========================================
- Hits 20033 20030 -3
- Misses 7707 7716 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ThinkOpenly
left a comment
ThinkOpenly
left a comment
There was a problem hiding this comment.
Neat! I think this is a good addition. @dhower-qc ? @lenary ?
If adopted, we need to annotate many other instructions, of course:
$ grep -l -E -i 'jump\(|jump_halfword\(|\$pc' $(find gen/resolved_spec/_/inst -type f)
gen/resolved_spec/_/inst/Smrnmi/mnret.yaml
gen/resolved_spec/_/inst/Zcmt/cm.jalt.yaml
gen/resolved_spec/_/inst/Zcmt/cm.jt.yaml
gen/resolved_spec/_/inst/Zcmp/cm.popret.yaml
gen/resolved_spec/_/inst/Zcmp/cm.popretz.yaml
gen/resolved_spec/_/inst/C/c.ebreak.yaml
gen/resolved_spec/_/inst/C/c.j.yaml
gen/resolved_spec/_/inst/C/c.bnez.yaml
gen/resolved_spec/_/inst/C/c.jr.yaml
gen/resolved_spec/_/inst/C/c.jal.yaml
gen/resolved_spec/_/inst/C/c.beqz.yaml
gen/resolved_spec/_/inst/C/c.jalr.yaml
gen/resolved_spec/_/inst/S/sret.yaml
gen/resolved_spec/_/inst/I/bne.yaml
gen/resolved_spec/_/inst/I/bge.yaml
gen/resolved_spec/_/inst/I/bgeu.yaml
gen/resolved_spec/_/inst/I/jalr.yaml
gen/resolved_spec/_/inst/I/blt.yaml
gen/resolved_spec/_/inst/I/beq.yaml
gen/resolved_spec/_/inst/I/ebreak.yaml
gen/resolved_spec/_/inst/I/jal.yaml
gen/resolved_spec/_/inst/I/auipc.yaml
gen/resolved_spec/_/inst/I/bltu.yaml
gen/resolved_spec/_/inst/I/mret.yaml
| vs: always # VS-mode access | ||
| vu: always # VU-mode access | ||
| data_independent_timing: true # For cryptographic extensions | ||
| pc: |
There was a problem hiding this comment.
| pc: | |
| pc: # program counter |
| references: true # Optional: instruction semantics reference the current PC | ||
| changes: false # Optional: instruction semantics update the PC |
There was a problem hiding this comment.
Subjective, but these might be better as read/write:
| references: true # Optional: instruction semantics reference the current PC | |
| changes: false # Optional: instruction semantics update the PC | |
| reads: true # Optional: instruction semantics reference the PC | |
| writes: false # Optional: instruction semantics update the PC |
If adopted here, these changes would be needed throughout this PR, of course.
| }, | ||
| "pc": { | ||
| "type": "object", | ||
| "description": "Explicit metadata describing whether the instruction reads and/or writes the program counter", |
There was a problem hiding this comment.
For consistency with other definitions here:
| "description": "Explicit metadata describing whether the instruction reads and/or writes the program counter", | |
| "description": "Whether the instruction reads and/or writes the program counter", |
|
I'm also not sure how a really weird instruction like |
Maybe nevermind on that concern. That instruction does not permit executing PC-relative instructions . |
|
Another way of implementing this would be adding an implicit operand to the instructions. The operand definitions are emerging in #1527. One nice thing that's already present there is the concept of "source" (read) and "destination" (write). Not yet present there, but probably needs to be is "implicit" operands. Program counter is certainly one. Stack pointer is another. |
dhower-qc
left a comment
dhower-qc
left a comment
There was a problem hiding this comment.
Not opposed to this, but would like to see a regression test that ensures IDL and the YAML annotation agree.
Signed-off-by: Aman <amkr6207@gmail.com>
|
Addressed the requested changes. I added the regression test, the doc comment, and the schema wording update. I left Thanks! |
3 participants
Fixes #1825.
Add explicit instruction-level metadata for whether an instruction references and/or changes the PC, rather than inferring that from
operation()IDL.This change:
pc.references/pc.changesfields to the instruction schemaauipcjalrmret