(Long) comments on riscv-v-spec-1.0.pdf

[L Peter Deutsch]

Overall comment: the content of this document looks great, but it needs some
detail cleanup, and there are a couple of content improvements I think are
important.

General comments
----------------

Some tables don't have a table number or title. The first two I noticed
were the vlmul table in Section 3.4.2 and the vta/vma table in 3.4.3, but
the entire document should be reviewed to check this.

Indentation of the instruction lists varies: I first noticed this in Section
13.2. I would advocate a pass to make it consistent.

Changes from v1.0-rc2
---------------------

"this it" should be "this is". :-)

Introduction
------------

In line with a set of as yet unpublished recommendations for extension
documentation that I've proposed, I advocate strongly adding a new
subsection, at least a couple of paragraphs, that discusses the rationale
for the significant aspects of this proposal that differ from the SIMD
instructions that are the basis for vector computation in the other ISAs I'm
familiar with. I seem to recall seeing this in some earlier draft, and I
think it would be very valuable to put it back in.

Section 3.1
-----------

A comma is needed after "v0-v31".

Section 3.2
-----------

In the last note, there should not be a comma after "is clear".

Section 3.3
-----------

The second and third paragraphs refer to "V=1". I don't know whether this
should be "vsstatus.V", "misa.V", or something else, but in any case it
should definitely include what register it refers to.

Section 3.4
-----------

In the first Note after Table 2, "ma and ta" should be "vma and vta".

In the last Note, I think "64-bit" should be "48-bit or wider".

Section 3.4.2
-------------

In the first Note, there is a reference to "(EMUL)", which isn't defined
until Section 5.2. I think it should be removed, but if not, I think a
reference to Sectoin 5.2 would be helpful.

Section 3.4.3
-------------

The third text paragraph ("When a set is marked agnostic") has several usage
issues. I suggest: "When a set is marked agnostic, the corresponding set of
destination elements in any vector destination operand can either retain the
value they held previously, or be overwritten with 1s. Within a single
vector instruction, each destination element can be either left undisturbed
or overwritten with 1s, in any combination; the pattern of undisturbed or
overwritten with 1s is not required to be the same when the instruction is
executed with the same inputs."

Section 3.6
-----------

In the second Note, I think "which require" should be "which would require",
or else "which requires".

Section 3.8
-----------

In the first paragraph, it isn't clear what "should be written as zeros"
means: writing non-zeros is required to trap, writing non-zeros may trap,
writing non-zeros may or may not read back the same value later, ....

Section 3.9
-----------

Same comment as for 3.8.

Section 5
---------

In the table of instruction formats, the entry for OPIVV is missing the 000
in bits 14-12.

Section 5.4
-----------

In "The active elements ...", "and where" should be "for which".
Similarly, in "The inactive elements ...", "but where" should be "for
which".

I found the last sentence of the final Note ambiguous: either all such
instructions do use a zero value for source indices greater than VLMAX, or
they don't. If they do, it should say "For indices greater than VLMAX, the
value 'read' is 0." If they don't, it should say something like "While the
value 'read' for indices greater than VLMAX is usually 0, some instructions
may behave differently, as described under the individual instructions
below."

Section 6.1
-----------

The table duplicates the one in Section 3.4 and its subsections. I'm not
sure how best to fix this, but I think it would be helpful to do so.

Section 7.2
-----------

In the first sentence, "base registers" should be "base addresses".

In the first Note, "is why" should be "is the reason that".

Tables 9 and 10 could be combined for less verbosity. E.g., the opcodes for
mop values 0 0 could be "VLE<EEW>, VSE<EEW>".

Section 7.3
-----------

I suggest that the last line of Table 13 should be named "Reserved
with vector width encodings", because for scalar FP, inst[28] is part
of the immediate offset and is not reserved.

Section 7.4
-----------

At the end of the last Note, "reducing need" should be "reducing the need".

Section 8
---------

In the Note, "one" should be "one of". And what is PMA?

Section 9
---------

Will the "more formal definitions" be included in this document?

Section 10
----------

Again, the 000 is missing for OPIVV.

Section 10.1
------------

The sentence "Use of the frm field ..." appears to be a leftover from an
earlier design in which some instructions had such a field, like the scalar
FP instructions: the previous sentence says that the rounding mode is always
taken from the frm register. I believe this sentence should simply be
deleted, or else replaced with something like "As for scalar FP operations,
if the value in the frm register is invalid, any vector floating-point
operation will raise an illegal instruction exception."

Section 10.2
------------

In the next to last Note, "was used on opcode" should be "was used on the
opcode", and "operations in doubleword" should be "operations on
doubleword".

Section 10.3
------------

In the first Note, "the belief the" should be "the belief that the".

Section 11.1
------------

"Integer adds." should be "Integer add".

Section 11.7
------------

The second Note seems to imply that the value in rs1 is not the shift amount
but either log2 of the shift amount (allowing only shifts by 2^N bits, which
seems unlikely) or the shift amount - 1 (which wouldn't be unreasonable, but
which I didn't see specified anywhere). If the example is correct, how the
vs1 / rs1 / uimm values specify the shift amount needs to be spelled out,
probably best in 11.6.

Section 11.8
------------

What is "PoR" in the Note about 2/3 of the way down p. 51?

Section 12.5
------------

In the last Note, "inbetween" should just be "between".

Section 13
----------

The discussion of mstatus and vsstatus duplicates Sections 3.2 and 3.3. I'm
not sure how best to resolve this.

Section 13.9
------------

In the next to last paragraph on p. 62, what is "B"?

I would like to see Table 16 packed down so it only occupies an 8 x 16 grid
rather than 3 full pages.

Section 13.10
-------------

Next to last paragraph on p. 66, what is "B"?

Same comment about Table 17 as Table 16.

Section 15.1
------------

Last text sentence on p. 77: "possibly binary" should be "possible binary".

It would be helpful to forbid a page break between the "inputs" and "output"
tables.

Section 18.2
------------

Where does this document say what happens on operations involving integer or
fixed-point scalars when EEW=64 and XLEN=32? I couldn't find it in the
initial sections, but I'm sure it must be there.

In the last paragraph "implement all" should be "implements all."

Section 19
----------

The load and store instructions are missing. I think a reference to Table
13 in Section 7.3 would be sufficient.

I would really like to see the column dividers in the tables lined up. It
took me a moment to realize that the first table (with headings "Integer",
"Integer", and "FP") was meant to be a key for the following tables, because
the column dividers were so far off.

Appendix A
----------

Sorry, I didn't try to read this.

Appendix B
----------

As a matter of security, I advocate strongly that the requirement for system
calls to either preserve or reset the vector state be made part of the
specification, not "Most OSes will choose to", even though this is just a
placeholder.

--

L Peter Deutsch <gh...@major2nd.com> :: Aladdin Enterprises :: Healdsburg, CA

Was your vote really counted? http://www.verifiedvoting.org

[Simon Ochsenreither]

To pile onto Peter's very useful list, I have four additional remarks based on the experience of reading the spec to build a compiler backend:

- I found the style of introducing instruction names mid-paragraph very confusing. My expectation was that the names are either part of the corresponding sections' titles, or (perhaps even nicer) as a side note.

- It's hard to read up upon specific instructions (which I do a lot when cross-referencing instructions between x86, Arm and RiscV), because the descriptions aren't self-contained, but implicitly built on all content preceding that page.

- I believe that some visual fine-tuning of the instruction encoding charts could substantially improve their readability.

- It seems that the width of the individual instruction encoding chart columns is intended to represent the amount of bits they encompass, but looking more closely this isn't the case.

Thanks,

Simon