-
Notifications
You must be signed in to change notification settings - Fork 40
Add spec to FP Computational and Conversion Instructions #810
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work!
Just a couple things to go:
Saying instruction in the instruction name seems clearly a no-go. Please remove from all the instructions long-names.
About the 64-bit vs Long, I think it's better to define long, but other reviewers may differ opinion.
description: | | ||
No description available. | ||
-id: inst-fcvt.h.lu-behaviour | ||
-normative: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is a requirement to enter, but since you specified, I believe all of this is normative text.
BTW, Normative text specifies the binding rules and requirements that implementations must follow (using terms like “MUST,” “SHALL,” or “SHOULD”), whereas non-normative text is purely informational—providing background, examples, rationale, or guidance without imposing any mandatory obligations on implementers. Normative clauses form the “contract” of a specification, and violating them means non-compliance, while non-normative sections merely aid understanding and carry no compliance weight.
Let’s see what @ThinkOpenly suggests too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my understanding, you have a lot of the "from" and "to" types reversed. I've suggested new text that reads better left-to-right with the mnemonic string and reverses those types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't all the descriptions normative?
As per @ThinkOpenly's guidance : |
That was my understanding at the time. I've since heard that what is actually ratified is not the text per-se, but the meaning of the text, so there is some flexibility with actual wording. That being said, I don't think we want to spend a lot of effort at this point fussing over wording and whether text is normative or not. Some meaningful text is helpful. Trying to be consistent is a good goal. Correctness is paramount. I expect there will be a future (substantial) effort to rectify text. |
arch/inst/D/fcvt.d.s.yaml
Outdated
-text: | | ||
The double-precision to single-precision conversion instruction, `fcvt.d.s` is encoded in the OP-FP | ||
major opcode space and both the source and destination are floating-point registers. The `xs2` field | ||
encodes the datatype of the source, and the _fmt field encodes the datatype of the destination. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encodes the datatype of the source, and the _fmt field encodes the datatype of the destination. | |
encodes the datatype of the source, and the `fmt` field encodes the datatype of the destination. |
arch/inst/D/fcvt.d.wu.yaml
Outdated
-id: inst-fcvt.d.wu-behaviour | ||
-normative: false | ||
-text: | | ||
This variant of the `fcvt` instruction converts to or from unsignedn integer values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo here "unsignedn", but this sentence can be removed entirely, as it doesn't really apply to this particular instruction.
arch/inst/D/fcvt.l.d.yaml
Outdated
@@ -3,9 +3,13 @@ | |||
$schema: inst_schema.json# | |||
kind: instruction | |||
name: fcvt.l.d | |||
long_name: No synopsis available | |||
long_name: Floating-point Convert Double-precision to Long integer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency, we need to either use "Long" or "Long integer". Let's use just "Long" everywhere.
arch/inst/Zfh/fcvt.h.w.yaml
Outdated
@@ -3,9 +3,12 @@ | |||
$schema: inst_schema.json# | |||
kind: instruction | |||
name: fcvt.h.w | |||
long_name: No synopsis available | |||
long_name: Floating-point Convert short to Half-precision |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long_name: Floating-point Convert short to Half-precision | |
long_name: Floating-point Convert Word to Half-precision |
arch/inst/Zfh/fcvt.h.wu.yaml
Outdated
@@ -3,9 +3,12 @@ | |||
$schema: inst_schema.json# | |||
kind: instruction | |||
name: fcvt.h.wu | |||
long_name: No synopsis available | |||
long_name: Floating-point Convert unsigned short to Half-precision |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long_name: Floating-point Convert unsigned short to Half-precision | |
long_name: Floating-point Convert Unsigned Word to Half-precision |
Just to clarify—“normative” and “ratified” are completely unrelated terms, right? In my understanding, ratified should simply indicate whether something has gone through the ratification process, not describe the nature of its content. |
IMHO, I see a "ratified" specification as describing compliant behavior/implementation, and the "normative" text therein as that which must be followed to be compliant. So, to me, they are related. (I'm content at this point to let correct text get merged, absent glaring omissions. Verbatim spec content is nice, but not required, and also difficult to contextualize when extracted. My criteria is "good enough". The text battles come later.) |
Agreed; I'm assuming there will be a deep pass of all text down the road, so it's not critical at this point. |
This PR adds missing long_name and description to some FP Computational and remaining FP Conversion Instructions.