-
Notifications
You must be signed in to change notification settings - Fork 4
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
Updates #3
Updates #3
Conversation
behavior: ACC = 0; | ||
} | ||
|
||
GET_ACC_LO { | ||
encoding: 7'd1 :: 10'b0 :: 3'd0 :: rd[4:0] :: 7'b0001011; | ||
assembly:"{name(rd)}"; | ||
assembly: {"s4e.get_acc_lo", "{name(rd)}"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I thought I understood any universal rule for how RISC-V instructions get named I'd be suggesting that a generation rule is placed elsewhere, but it would surely need the possibility of overriding per-instruction anyway.
As I remember it, the assembly statement was added to CoreDSL to support disassembly in the MINRES tooling and gets concatenated into a Python string (hence the "name" array of integer registers, supplemented in the FP instructions with the "f" array. If some directly addressable custom registers get added through an extension then some more generic way of labeling them will be needed). So an extension written using this approach would probably break the MINRES tooling - given how under-specified the "assembly" statement is this isn't an immediate issue, but maybe an additional "instruction" statement would be safer than extending what's there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed earlier. This mnemonic syntax is part of the CoreDSL spec and supported by the MINRES tooling. It is entirely optional but Seal5 can make use of it. The extensible compiler will not understand it but there is no need to fix this as an older commit of this repo can still be used.
Could we please merge this soon @thomasgoodfellow ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW the syntax change happened here: Minres/CoreDSL#80 (comment)
No description provided.