-
Notifications
You must be signed in to change notification settings - Fork 41
Add sub-types to existing instruction formats #655
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
Comments
@Unique-Usman can we discuss this topic here from now on? Please comment this so I can assign it to you |
Hi @AFOliveira, Got stuck in college activities. I am back now. I suggest we use this Github thread instead of excel. People Can also see it and easily drop comment here ? What do you think ? |
That's fine, but how? 1000+ lines is not readable as you posted, so maybe you will need to format it in github, which may be a lot of work. |
I think we should use Divide and Conquer approach, I think we should go extension by extension as some of the instruction subtypes are similar across extension. what do you think? |
Problem is that won't be easy even for people who may come just to review. Also, the same sub-type may exist in several extensions, but I think it's your call, just try to make it readable, and I'll try to review and ask people to review it |
Yeah, Subtypes exist in different extensions actually. Maybe we can have it in excel. The question is, how do we make it synchronized. For excel, if we have everything inside the excel file, people can come randomly and review. It will be add to know what has been reviewed and what has not been ? If we have in a thread, we can go extension by extension. Have discussion on it, then go to the next extension. If someone comes they can view what has been discussed and proposed. And they can dropped their own idea. Will this be easy to do in excel ? |
This is the sample of what I am saying for example starting with A Extension A - ExtensionR-aq-rl type - suggested type
R-aq-rl-ls type - suggested type
|
We can have people, review the above A extensions and the suggested type. After it has been reviewed, we can now add it to the excel or some other file extension. We can go four extension instead of a single extension for faster review. @AFOliveira what do you think ? |
As a reference that I find useful, here is a representation of one of the top-level instruction formats supported by the OpenPOWER ISA: I like how the subtypes are all listed together in a compact table, so you can easily see the differences among them. @Unique-Usman: pick an extension that has a small to moderate number of subtypes, and within the list of that extensions instructions, pick just one top-level format, say "I-type" or "R-type". Create a table similar to the image above, and let's review that. These tables should probably have a list of associated instructions (from just the one extension) with each row. This information will make the review much easier. Then we can add, one-by-one, the additional top-level formats covered by that one extension. Then, we can move, one-by-one, to the other extensions. How does that sound? |
@ThinkOpenly, that sounds very good. I think we can create the table using the excel format. The question now is where the I post the table for review and discussion here or in the excel format (I do not think this is easier or possible though)? |
Github allows the following syntax, but I think you will have to see whatever is easier for you and reviewers
|
@AFOliveira, that is a good approach. Thanks. |
RISC-V B Extension Instruction FormatsR-type (Suggested-name -> R-default)
I-type (Suggested-name -> I-shamt-32)
I-type (Suggested-name -> I-shamt-64)
I-type (I-rev8)
Above is for B-extension, I am not sure if rev8.yaml is I-type or R-type as I could not find it anywhere. Also, the andn insstructions and other in the first have the same structure as mentioned in the ISA for R-type so, that is why I suggested R-default for them. |
Great start! Now, my review:
R-type should stay R-type, we do not want to change ratified spec.
Agree with both.
Isn't this exactly the ratified I-type? It should stay I-type. Also, this appears in more instructions than just those two, like the register-immediate instructions. |
Noted. The default should not be changed.
Yeah, thanks for catching this. I missed it. I will update the script. The default should be default. |
Two minor requests/advice for future discussions:
Agreed!
Are they? They don't use the "imm" field as an immediate value which is used in the instruction operation. Here, it's really another segment in the full opcode. I don't know if there are others like this such that the "I-rev8" is too specific to these instruction encodings. I suggest "I-opcode" for now. This content was much, much easier to review, so thanks for the extra effort, @Unique-Usman ! Should we create a PR that adds the "subtype" capability to the instruction schema, then we can start merging these "approved" sets of instruction subtype classifications into the database? As long as the "subtype" field isn't made "required", we can begin pushing them in a bit at a time. |
Noted. I will take that into account. Thanks.
There are few other instructions same as I-rev8/I-opcode in other extension. I think I was the one who got it confused. I shouldn't have used imm[11:0] to represent those bit from 31-20. I think we can use const or something. As the byte from 31-20 is constant and already specified. We can call it I-opcode.
A good suggestion. |
Let's call it "opcode2" for now. Or, maybe "funct11" is better, given the way the other similar "funct*" fields are named.
@Unique-Usman, are you able to proceed in creating that PR? If so, do let us know if you need any assistance. |
I think funct11 is okay and similar to other similar funct*
Adding something like this "subtype": { after "$source" in the inst_schema.json ? |
A few comments:
File arch/inst/sll.yaml # ...
encoding:
RV32:
subtype: { $ref: inst_subtype/i_shamt_32.yaml# }
# ... File arch/inst_subtype/i_shamt_32.yaml # making this up, open for debate
variables:
shamt:
location: 24-20
type: shamt
rs1:
location: 19-15
type: xsrc
rd:
location: 11-7
type: xdest
opcodes:
funct7:
location: 31-25
funct3:
location: 14-12
opcode:
location: 6-0 Thus, for the schema, we need to add the reference to inst_schmea.json (see riscv-unified-db/schemas/inst_schema.json Lines 217 to 224 in cd2b7ea
|
Yes, I was thinking we add two attributes to each instruction: format and subtype, with "format" being the primary classification, as in the spec, so "R", "I", "S", "U", etc. (from chapter 2.2 "Base Instruction Formats").
Here, I'd vote for
Right... much of which would be copied from the existing schema for instruction fields. |
👍 |
Thanks @dhower-qc and @ThinkOpenly. I tried to come up with the update to the schema inst_schema.json but, I think I need to take a closer look into how the schema is being done as I am not that familiar to it. |
I already did that, don't worry about it, I'll PR it to UDB tomorrow. |
Really good question! The spec also says:
So, the registers are exactly the same registers, but the opcodes for double precision operations will treat the values as double-precision (64-bit) values. Since the registers are the same, all else being equal, there is no need for different subtypes between single-precision and double-precision instructions. |
Oh, so basically, for previous instructions we are making the distinction based on the register type i.e integer or float. Double and float uses the same registers ? e.g
|
That's right; integer and FP use different register files, so there is no similarity between |
Hello, Let's continue from where we stopped. I-type
The rs1 here is integer and the rd is floating point register. Should we call this I-d-fi Subtype ? S-type
The rs1 here is integer and the rs2 is floating point register. Should we call this S-d-fi Subtype ? |
Remind me, for "I-d-fi":
|
Yeah, I am doing this just to have some sort of semantic meaning for the subtype name but, it is not hard requirement. |
Maybe I'm missing something, but does that mean instructions from the F/D/Q/Zfh extensions will all be different types? And if so, what's the justification for that? Seems like |
hmm, that is really a good point. One approach is that we could remove the extension part, I also notice that some instructions do have changes in their extension . Another one is we could leave it but, without semantic meaning or not linked to the extension. What do you think ? |
The subtypes are really about the fields and their respective interpretations. It could be that we crisply define the types in some fashion, and the subtypes become obvious. I'm picturing a CSV were the fields are laid out [(location, type), (location, type), ...], then you just If we focus on the fields, taking the most recent, FLD, as an example:
Maybe "I-i-x-f" (Type-field0-field1-field2-..., ignoring the non-variables, unless they don't conform to expectations in some way, like "rounding mode" perhaps)?
For FSD:
Maybe "S-i-f-x"?
|
This looks good and I think it is well thought out. |
It means we have to review and rename all the initial subtype in D extension |
I vote yes. I feel like we're iterating to get to a good solution, and this is expected with a task this large. Thanks for sticking with it! |
Sorry for being late, but I agree with the new naming, seems like a reasonable step! |
+1 from me |
My suggestion for B extension. RISC-V B Extension Instruction FormatsIt is already R-type
Suggested type -> I-s-x-x-32
"I" — It's an immediate-type layout Suggested type -> I-s-x-x-64
"I" — It's an immediate-type layout I-x-x
"I" — Instruction uses I-type field layout |
I am sorry for the late communication, I went back to my country Nigeria for summer holiday. And it was a long and tiring journey. |
Don't worry about that @Unique-Usman! Naming seems good to me! |
@ThinkOpenly @dhower-qc @AFOliveira , let also not forget the other issue here #686 |
I probably should've shared this earlier, but it just occurred to me that this would be a helpful resource. As part of developing the These types can all be found as the second column of our CSV test plans (https://github.com/openhwgroup/cvw-arch-verif/tree/main/testplans). Taking a quick look, we seem to have similar groupings so far just with different names. |
@ThinkOpenly @dhower-qc @AFOliveira, you guys can take a look, it looks interesting and maybe we could use something from there. |
I'll be taking a look, thanks @jordancarlin . In the meantime @Unique-Usman, check if all matches to what you are doing, and you may use that a starting point, easing the effort of your task |
Thanks @jordancarlin. There is definitely a lot of overlap. I think the general statement is that a type in cvw maps to at least one unique UDB subtype. We are trying to differentiate on a few more properties, which is going to result in a few more splits. @Unique-Usman, that's definitely a good resource. If you see something in cvw that has two types where we only have one, then we need to split the subtype in UDB. |
Yeah, there are some extension in the cvw not in UDB, e.g there https://github.com/openhwgroup/cvw-arch-verif/blob/main/testplans/Zbkc.csv, which has two instructions and it is gotten from B extension, do we create that also in UDB ? |
Zbkc exists in UDB |
Yeah, that is true, it is in the definedBy. |
Let's continue from where we stopped. @dhower-qc and @ThinkOpenly, can you take a look at this ? |
looks right to me |
Let's move to D extension. For the first part.
For rs1 and rs2 are floating point registers and rd is also floating point registers (double) Fsgnjn.d, fmaxm.d, fminm.d, fmin.d, fsgnjx.d, fmax.d, fsgnj.d, fsgnj.d
For rs1 and rs2 are floating point registers(double) and rd is integer Fltq.d, fle.d, feq.d, flt.d, fleq.d
Integer registers rs1 and rs2 and the rd is float (double) fmvp.d.x |
Should we put the 32/64 next to the |
### Add instruction “type” schema & link sub‐types (closes #676) - **inst_schema.json**: - Introduce `type` object under `encoding` with a `$ref` to external `inst_type/...#.yaml` files - **inst_type_schema.json**: - Import of encoding schema for defining RV32/RV64 encodings for instruction types - Lays groundwork for sub‐types in R/I/B/S/U formats as discussed in #655 --------- Signed-off-by: Afonso Oliveira <[email protected]> Co-authored-by: Derek Hower <[email protected]>
Summary
Track progress on introducing sub-types for our R-, I-, B-, S-, U-type instruction formats to improve clarity, tooling and testing.
Motivation
Proposal
subtype
field to our instruction schema.R.aqrl
— acquire release RtypeNext Steps
The text was updated successfully, but these errors were encountered: