Support RVC hint instructions rather than trap #31
Conversation
| mapping clause encdec_compressed = C_SRLI64(rsd) | ||
| <-> 0b100 @ 0b0 @ 0b00 @ rsd : cregidx @ 0b00000 @ 0b01 |
There was a problem hiding this comment.
Shouldn't this (and the other new C_x64 instructions below) have a guard on the architecture being RV128 (i.e. sizeof(xlen) == 128) similar to the guards in SHIFTIOP?
There was a problem hiding this comment.
No, they're hints in RV32/RV64.
There was a problem hiding this comment.
(the exception is RV128, where an immediate of 0 expands to srli rd, rd, 64 etc)
There was a problem hiding this comment.
It would have been nice to have an explicit indicator for hints, and removing the guards in this way hides the hint variants. On the other hand, trapping is bad. Perhaps we should add a comment indicating the hint case where the hint guards have been removed.
|
We don't do that for all the various ways of writing NOP in the base instruction set, which also has a well-defined set of reserved for standard use and reserved for custom use hints. I think it would be misleading to add that for the compressed case. |
|
The spec has statements like "C.ADDI is only valid when rd/=x0 and nzimm/=0." When the spec is annotated with the Sail latex, it would be odd and confusing to see that statement not represented in some form in Sail. |
|
I think that’s a flaw in the spec though as it’s hard to reason about the validity of the hints. It should read the same as the base instruction set, which doesn’t say “ADDI is only valid when rd /= x0”. Such statements should be reserved for when it’s an illegal instruction, or a completely different instruction. I agree there’s a disparity now between the Sail and the prose part of the spec, but I’d argue that’s a deficiency in the spec. |
|
I agree that it is a deficiency in the phrasing of the spec. Could you open an issue on the spec repo explaining this? I'm reluctant to have Sail diverge from the prose spec, and would rather see that spec fixed first. |
|
Ok, I will do so tomorrow. |
|
On Tue, 7 Jan 2020, 23:49 Prashanth Mundkur, ***@***.***> wrote:
The spec has statements like "C.ADDI is only valid
What does that "valid" mean exactly?
when rd/=x0 and nzimm/=0." When the spec is annotated with the Sail latex,
… it would be odd and confusing to see that statement not represented in some
form in Sail.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#31>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFMZZSAKSND4JRRMPU4HN3Q4UIHVANCNFSM4J6YVH3A>
.
|
|
Was there ever an issue opened on the ISA spec? I can't find anything there. |
|
Could I close this given the hint support now in master? |
|
Well the assembly for those is wrong, and the SRLI is called SRLI64 in the spec. |
@jrtc27 Do you know what the status of this is? It's been sitting here for a while. |
Unchanged; the implementation is correct behaviour-wise as far as I recall, but the disassembly isn't right. |
How about we close this PR and reopen, when this is fixed? @jrtc27 |
* update riscv_arch.h to support QEMU: add size attribute for tohost/fromhost add writing zero to (tohost + 4) to write_tohost * revert the align of tohost/fromhost to 64 bytes
No description provided.