Allow ELEN>VLEN in Vector extension #2721
Conversation
Normative Rule Changes DetectedThis PR modifies normatively tagged text. Please review the changes below to ensure they are intentional. View Detected ChangesNormative Tag Change ReportUnprivileged SpecificationPrivileged SpecificationWhat happens next:
How to update reference files (if needed): make update-ref
git add ref/*.json
git commit -m "Update normative tag reference files"
This comment was automatically generated by the normative tag check workflow. |
|
This change seems actual revision to hardware-software contracts. Will it bump the version of V extension to 1.01, 1.1, 2.0 or whatever? |
aswaterman
left a comment
There was a problem hiding this comment.
My recollection from having discussed this with @kasanovic is that it will come in the form of new extensions, not revisions of the existing ones.
|
@aswaterman , indeed new extensions will be required to specify the configurations; existing "V" and zve* remain intact). But the baseline specification must allow that (as of now it directly prohibits). We talked about that with @kasanovic and in Vector SIG. But of course there is a question how to best put that to the spec. I am not sure that my version is the best possible, certainly this requires a thorough review, and I'd appreciate any feedback/suggestions. |
|
@DmitryUtyansky I will provide more feedback sometime down the road, but at first blush this appears to be on the right track (modulo the extensionology matter). |
|
@omasanori , I am not sure what's the best approach with the versioning. The extensions defined in the document (V, zvl*, zve*) are not changed (at least that's the idea, unless I messed up somewhere). Existing contracts shall stay intact. But the text is changed anyway. Alternatively, one could think of more clustered/localized changes, giving the updated formulas only for ELEN>VLEN cases and leaving the existing paragraphs intact. But overall document is then changed all the same. |
Co-authored-by: Craig Topper <craig.topper@sifive.com> Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
Drop stores, since only whole register load instructions have the element size encoded. Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
Co-authored-by: Craig Topper <craig.topper@sifive.com> Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
There was a problem hiding this comment.
Seems like we're on the right track.
Since we are globally relaxing the ELEN <= VLEN requirement, we should explicitly change the definitions of the Zve32x etc. extensions to require ELEN <= VLEN. That keeps the existing extensions functionally unchanged.
|
Table 60 in Section 30.18.2 already explicitly lists the ELEN <= VLEN requirement for each extension. |
|
Ah right, thanks. |
More concise, "vector register (or a vector register group)" => "vector register group" Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
Whole register loads and moves EEW simplified Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
Non-normative explanations of changes and that nothing is changed for ELEN<=VLEN throughout the document have been removed and put instead into one Note in the beginning. Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
|
@aswaterman , I moved the explanations into one big note in the beginning, and did other small edits. Now I believe the doc is good for review, please take a look. |
aswaterman
left a comment
There was a problem hiding this comment.
Thanks, I think this is getting close. Ping me after this round of feedback is addressed.
Misc. small syntax corrections & rephrasing according to the feedbacks. Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
Unify spacing around = in formulas. Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
More spaces around = unification Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
Use ELEN > VLEN conditions instead of ELEN/VLEN > 1 Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
Adopting the more concise formulation of no impact on ELEN <= VLEN cases. Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
aswaterman
left a comment
There was a problem hiding this comment.
@DmitryUtyansky Thank you for hanging in there with my review; I think this is good to go after you address my last minor comment. But I'd like to give @kasanovic the chance to weigh in.
Minor rewording of mapping for LMUL > 1 Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
| The load instructions have an EEW encoded in the `mew` and `width` | ||
| The load instructions have the element width encoded in the `mew` and `width` | ||
| fields following the pattern of regular unit-stride loads. | ||
| EEW is computed as EEW=min(VLEN, EEW_encoded). |
There was a problem hiding this comment.
EEW is used as a hint to microarchitecture and doesn't affect architectural behavior of these instructions - I don't think it should change in case some funny trick is used by micorarchitecture knowing that EEW > VLEN?
There was a problem hiding this comment.
I think this is a clean definition that doesn't actually preclude funny tricks. I suggested it because it's important we don't preclude vtype-unaware register spill/fill code from e.g. moving v1 to v2 when SEW > VLEN. Without this definition, the reference to v1 would make the instruction reserved.
A uarch is free to ignore this dictum in its trickery and represent the destination with whatever EEW it wants, e.g. it could compute its internal-representation EEW as a function of VLEN, EEW_encoded, and the register specifiers involved.
There was a problem hiding this comment.
I deleted there the paragraph left by oversight after the earlier edit introducing "EEW=min(VLEN, EEW_encoded)." ("In implementations supporting ELEN > VLEN, element size can exceed the number of bits available in a vector register or a vector register group. In that case, the whole vector register load instructions
still operate on the specified number of vector register(s), using the
least-significant bits of the element.")
It's either one or the other, EEW as min(...), as @aswaterman suggested or "use LSBs of an unchanged bigger EEW" as it was originally.
The benefit of having EEW defined through min(...) is that the subsequent formulas evl=NFIELDS*VLEN/EEW works without giving fractional evl.
Thinking more about this, the formula with min(VLEN, EEW_encoded) for e.g. VLEN=32 EEW=64 breaks e.g. vl2re64.v: a perfectly valid "load pair of vregs with EEW=64" is now switched to EEW=32 (potentially misguiding all those "microarch hints").
I have changed the formula to "EEW=min(VLEN*NFIELDS, EEW_encoded)", limiting EEW to whatever fits into the requested group. Same logic applies to whole register move: I have changed the formula there as well, to EEW = min(VLEN*EMUL, SEW), factoring in EMUL.
@aswaterman , @kasanovic , please tell me what you think of the current wording.
There was a problem hiding this comment.
Good catch. I think this scheme hangs together. I'll chat with @kasanovic about this topic later this week and try to reach a conclusion.
kasanovic
left a comment
There was a problem hiding this comment.
I think this is mostly OK but there are a few tweaks in wording , extension->implementation, and also the whole register move to update.
"implementation" instead of "vector extension" in various non-standard examples, small edits, change of formulas for whole vector register loads and moves to account for EMUL while limiting EEW. Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
|
@aswaterman , @kasanovic , so what's your opinion, does this require any further refinement, what's the next step(s)? |
|
Sorry, I'll try to re-review this before the end of the month. I think it is basically OK, but I want the chance to think it through carefully again. |
aswaterman
left a comment
There was a problem hiding this comment.
I think this is ready to go after my minor comments are addressed. Please ping me once you've done so.
Also, after merging, please delete the riscv/riscv-isa-manual-dmitry repo. (In the normal github workflow, forks should be in your personal github area rather than the original one.)
A couple of corrections (semicolon, extra parenthesis). Signed-off-by: DmitryUtyansky <118922093+DmitryUtyansky@users.noreply.github.com>
Thanks for getting back to this and spotting! I did the corrections. |
Change/amend wording and formulas in Vector extension to allow ELEN>VLEN configurations, along the lines agreed upon in Vector SIG. To allow e.g. ELEN=64, VLEN=32 extensions.