search

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
53 stars 23 forks source link

Ambiguous ID requirement description #144

Closed flegert closed 4 months ago

flegert commented 4 months ago

The definition of the allowed ID values contains the following snippet:

The id values are required to be incremental wrapping for sequential instructions, but do not necessarily need to be continuous.

The sentence leaves room for interpretation. "Sequential instructions" should be clearly defined (Sequential in a sense that no native CPU instructions are in-between? Sequ. from CPU perspective? ...) Due to the way the sentence is written it could also be misinterpreted which IDs can be "continuous" and which don't have to be.

Maybe it would also help to clearly define what is meant with "incremental wrapping" and how potential overflows of IDs have to be handled.

christian-herber-nxp commented 4 months ago

Thanks for the input. I am willing to update to clarify, but I am not yet sure how. The IDs are a value of the extension interface, so by saying they do not need to be incremental, it is meant that the IDs used when offloading via the issue interface do not have to be continuous between two sequential offloads. Incremental means that ID numbers increase. Wrapping means you start from zero when you reach the maximum value. Maybe this helps you to make a suggestion for a change or clarification in the text.

flegert commented 4 months ago

Thank you for the explanations. One potential improvement I would suggest might be something like this:

The id values are required to be incremental wrapping for sequential instructions, but do not necessarily need to be continuous otherwise / between sequential offloads.

This would prevent possible misunderstandings (without it one might interpret the id values to be "incremental wrapping" but "not continuous" at the same time)

For the term "sequential instruction", a note might help. I'm not sure how I would formulate that either, but maybe something along those lines but more formal:

Note: Sequential instructions relate to offloaded instructions that appear in order without any intermediate instructions recognized by the CPU.

christian-herber-nxp commented 4 months ago

Now I am not sure if we are on the page. By non-continuous, it is meant that the increment does not have to be 1.

flegert commented 4 months ago

As I understood the definitions, non-continuous means increments by 1, 2, 3, ... are possible, while incremental wrapping means only increment by 1, and go from 0x...FF -> 0x...00.

As I understood the sentence: "For sequential instructions, the id value must be incremental wrapping (-> and therefore must also be continuous). If the instructions are not sequential (e.g., if inbetween there is an instruction handled solely by the CPU), the id value can be non-continuous."

I hope I did not interpret that wrong.

If it is correct, than in my opinion the sentence still might be interpreted in other, self-contradicting ways, e.g.: "The id values for sequential instructions must not be continuous but must be incremental wrapping."

To put it short, the sentence seems correct but may be misleading. That's why I suggested adding "otherwise" or "between sequential offloads" at the end and separatly define "sequential instruction" to leave less room for such wrong interpretations.

Feel free to object though, maybe I'm overemphasizing this, also I don't want to waste your time on topics that may be too minor right now.