riscvarchive / riscv-code-size-reduction

https://jira.riscv.org/browse/RVG-122
150 stars 34 forks source link

Add proposed instruction formats #204

Closed tariqkurd-repo closed 1 year ago

tariqkurd-repo commented 1 year ago

Here's my proposal for the instruction formats based on the discussion in issue 192

tariqkurd-repo commented 1 year ago

as the PDF build failed - here's the rendered table. I think f1 should be funct1 maybe?

image
jalobaba commented 1 year ago

Tarik, I repeated what I came up with in early Nov 2022 in #192 below.

James Zc Format Proposal

Comparing Tariq's proposal to the above, I see that I have fewer formats because I took Andrew's suggestion in #192 to not create a new format if there are just register usage differences between instructions. This is how it was done in the C extension so there is precedent.

Here's a comparison between Tariq's proposal and mine:

  1. I've combined c.lbu/c.sb into one ZCB format and c.lhu/c.lh/c.sh into one ZCH format
  2. I re-used the existing C extension CA format for cm.mv* and put c.mul in there. Note that the latest RC candidate for the Zc spec says "The c.mul encoding uses the CR register format along with other instructions such as c.sub, c.xor etc." but this is incorrect because c.mul uses the CA register format, not CR. CR has 2 5-bit register fields but CA has 2 3-bit register fields. I think this mistake should be fixed in the Zc spec.
  3. Tariq, you have a 2-bit register field in bits 3:2 but it should be a 3-bit register field in bits 4:2. This creates several other errors as well since you've got 1 too many bits in the field left of 3:2.

Other than what I've listed above, both proposals are the same except for minor naming differences of fields (I'm by no means attached to the names I've come up with).

tariqkurd-repo commented 1 year ago

@jalobaba yes I need to fix that field - don't have time today but will soon. for the register fields, the existing formats e.g. CL and CS are distinguished by the register type - so load or store, so i think it makes sense to continue in the same way. For c.mv* even though the register field is the same number of bits, the actual meaning is different as it's an s register, which follows the distinction between CL and CS.

So my reading of it is that this is more consistent with the existing formats. what do you think?

jalobaba commented 1 year ago

Tariq, I think your interpretation is correct. I was misinterpreting the existing C instructions such as those that use the CR format because the register field is used as both source and destination so it is listed as rd/rs1. I was thinking rd/rs1 meant that some CR instructions use the field as just rs1 and others just as rd which isn't the case.

I don't see c.mul in your list of formats and missed the note at the bottom until just now.

I think Andrew is a reviewer. Maybe he'll have some good feedback on the formats?

aswaterman commented 1 year ago

I’ll try to take a look next week.

tariqkurd-repo commented 1 year ago

I've fixed the problem with the field widths which @jalobaba pointed out. If you look directly at the adoc file it renders the table (at the bottom of the file)

https://github.com/riscv/riscv-code-size-reduction/blob/fix_issue_192/Zc-specification/Zc.adoc

jalobaba commented 1 year ago

Looks good. How about renaming CMVSA to CMV2? +james+

On Monday, January 2, 2023 at 02:59:55 AM PST, Tariq Kurd ***@***.***> wrote:  

I've fixed the problem with the field widths which @jalobaba pointed out. If you look directly at the adoc file it renders the table (at the bottom of the file)

https://github.com/riscv/riscv-code-size-reduction/blob/fix_issue_192/Zc-specification/Zc.adoc

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

abukharmeh commented 1 year ago

Looks good. How about renaming CMVSA to CMV2? +james+ On Monday, January 2, 2023 at 02:59:55 AM PST, Tariq Kurd @.> wrote: I've fixed the problem with the field widths which @jalobaba pointed out. If you look directly at the adoc file it renders the table (at the bottom of the file) https://github.com/riscv/riscv-code-size-reduction/blob/fix_issue_192/Zc-specification/Zc.adoc — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.>

Yes, I agree or perhaps CDMV as in double move ?

tariqkurd-repo commented 1 year ago

The existing register specifiers werent consistent - for example for c.zext.b on the instruction page I had rsd' but in this table I had rd'/rs1'. This is the same for all the sign/zero extensions and c.not/c.mul

I've also used r1s' instead of sreg1 for cm.mv* which I've made consistent and update the PR

tovine commented 1 year ago

Looks good. How about renaming CMVSA to CMV2? +james+ On Monday, January 2, 2023 at 02:59:55 AM PST, Tariq Kurd @._> wrote: I've fixed the problem with the field widths which @jalobaba pointed out. If you look directly at the adoc file it renders the table (at the bottom of the file) https://github.com/riscv/riscv-code-size-reduction/blob/fix_issue_192/Zc-specification/Zc.adoc — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: _@_._>

Yes, I agree or perhaps CDMV as in double move ?

I don't know how I feel about this suggestion, as CDMV or CMV2 implies that it can move two arbitrary registers while in reality it's not that flexible (only s registers can be targeted, and only to/from a0 and a1).

tariqkurd-repo commented 1 year ago

thanks for the feeback. Any more? Are we happy with the CMVSA name? I think it makes it fairly clear what it's for.

aswaterman commented 1 year ago

I suggest soliciting feedback on the mailing list, too.

stayforapple commented 1 year ago

Could it work for the new Zve extension ?

jalobaba commented 1 year ago

I still think CMVSA is too cryptic and also biased towards one of the two instructions the format supports (i.e. cm.mvsa01 vs. cm.mva01s). How about CMVA0A1 since the 2 instructions using the format are always moving a0 & a1 to or from other registers?

tovine commented 1 year ago

I still think CMVSA is too cryptic and also biased towards one of the two instructions the format supports (i.e. cm.mvsa01 vs. cm.mva01s). How about CMVA0A1 since the 2 instructions using the format are always moving a0 & a1 to or from other registers?

Would CMV2A or something like that work?

tariqkurd-repo commented 1 year ago

I've gone for cmmv as it's the common prefix of both instructions, is distinguished from c.mv because of the extra m, and is 4 letters which make it look more consistent with the others.

What do people think?