openhwgroup / core-v-xif

RISC-V eXtension interface that provides a generalized framework suitable to implement custom coprocessors and ISA extensions
https://docs.openhwgroup.org/projects/openhw-group-core-v-xif/en
Other
60 stars 24 forks source link

Strange behaviour specified when compressed instructions are managed by coprocessor #193

Closed MPEZZIN closed 6 months ago

MPEZZIN commented 6 months ago

Comment

What is the reason that led to the bold text hereafter ?

The core shall attempt to offload instructions via the issue interface for the following two main scenarios:

  • The instruction is originally non-compressed and it is not recognized as a valid instruction by the processor's non-compressed instruction decoder.
  • The instruction is originally compressed and the coprocessor accepted the compressed instruction and provided a 32-bit uncompressed instruction. In this case the 32-bit uncompressed instruction will be attempted for offload even if it matches in the processor's non-compressed instruction decoder.

I don't see why it is mandatory to forward uncompressed instruction to the issue interface in that case. I think it would make sense to implement a coprocessor that is only in charge of compressed instruction expansion.

On the other hand, this would create a particular case for the following rule:

In case that both the processor and the coprocessor accept the same instruction as being valid, the instruction will cause an illegal instruction fault upon execution.

Proposed Resolution

In my humble opinion, the processor should never forward an already supported instruction to the extension interface, and this case should never occur:

In case that both the processor and the coprocessor accept the same instruction as being valid, the instruction will cause an illegal instruction fault upon execution.

Addition Info

No response

christian-herber-nxp commented 6 months ago

This statement predates my involvement with the spec. My assumption is that this was put in place for instructions that only have a 16-bit encoding, but no 32-bit encoding. This way, it is possible to support this without actually introducing a 32-bit instruction. @Silabs-ArjanB can you confirm?

MPEZZIN commented 6 months ago

My assumption is that this was put in place for instructions that only have a 16-bit encoding, but no 32-bit encoding

Oh I see... I didn't know that such instructions would exist in the standard extensions.

If I understand well the current proposal, compressed interface purpose is only to expand compressed instructions to their 32-bits counterpart, right ? So if we have to deal with such case, we have to introduce an intermediate custom instruction, that will not be recognized by the processor and will be forwarded to the issue interface.

Please correct me if I'm wrong.

christian-herber-nxp commented 6 months ago

yes. These kinds of instructions do actually exist in Zc. But of course you would also be allowed to do this for custom instructions. I will make sure to add a note describing the use case.

Silabs-ArjanB commented 6 months ago

My assumption is that this was put in place for instructions that only have a 16-bit encoding, but no 32-bit encoding

It has nothing to do with that. The reasoning is simple: If a coprocessor replaces a 16-bit compressed instruction by some 32-bit code then it shall be willing to then deal with that exact 32-bit encoding later on. Of course if would be weird if a coprocessor accepts and translates a 16-bit code to say a regular ADD instruction and then accepts that ADD instruction via the issue interface and assigns a new semantics to it, but this is exactly what we want to support however. A coprocessor shall simply not assume that it can translate something to a 32-bit encoding and expect the CPU to further handle it (otherwise we need to specifiy exactly which 32-bit opcodes a CPU needs to be able to handle).

MPEZZIN commented 6 months ago

@Silabs-ArjanB , I completely agree with your explanation, but I'm a bit surprised by the specified behaviour for the interface. I think we could get the same results with a simpler specification:

Silabs-ArjanB commented 6 months ago

Your explanation does not match the intended behavior. Specifically the part stating "if the processor is able to manage the uncompressed instruction provided by the coprocessor, then it is executed the normal way and the coprocessor issue interface is not triggered" differs. I agree the difference only matters in extreme corner cases.

christian-herber-nxp commented 6 months ago

My assumption is that this was put in place for instructions that only have a 16-bit encoding, but no 32-bit encoding

It has nothing to do with that. The reasoning is simple: If a coprocessor replaces a 16-bit compressed instruction by some 32-bit code then it shall be willing to then deal with that exact 32-bit encoding later on. Of course if would be weird if a coprocessor accepts and translates a 16-bit code to say a regular ADD instruction and then accepts that ADD instruction via the issue interface and assigns a new semantics to it, but this is exactly what we want to support however. A coprocessor shall simply not assume that it can translate something to a 32-bit encoding and expect the CPU to further handle it (otherwise we need to specifiy exactly which 32-bit opcodes a CPU needs to be able to handle).

@Silabs-ArjanB but then I do not understand why this would even be specified:

In this case the 32-bit uncompressed instruction will be attempted for offload even if it matches in the |processor|'s non-compressed instruction decoder.

You only need to prioritize the coprocessor if you assume that CPU is also able to execute the instruction. This is why I made that assumption.

MPEZZIN commented 6 months ago

You only need to prioritize the coprocessor if you assume that CPU is also able to execute the instruction. This is why I made that assumption.

OK, I see the point. But why should we do this for compressed instructions only ? From my point of view it should be always or never. There's no reason to assume it will have better performance than the processor.

christian-herber-nxp commented 6 months ago

Conclusion: unccompressed: Keep as is: must be decoded by CPU, may be offloaded to Copro if legel by the CPU. If illegal in CPU, must be offloaded to copro decompressed: must be offloaded, must be decoded by main CPU. Both accept -> illegal instr