Add Support for Zmmul #122

bilalsakhawat · 2021-11-03T13:25:11Z

RISC-V Zmmul extension includes following instructions:

mul
mulh
mulhsu
mulhu
mulw (RV64 only)
The opcode encodings and instruction semantics are identical to those in the M-extension.
The ISA string is Zmmul.
8.3 Zmmul Extension, Version 0.1 (riscv unpriv spec)

scottj97 · 2021-11-03T17:15:08Z

Should the new if haveMulDiv() qualifiers have been there all along? If so, can you make a separate commit with only that change, then a second commit to add Zmmul?

bilalsakhawat · 2021-11-03T17:31:10Z

Should the new if haveMulDiv() qualifiers have been there all along? If so, can you make a separate commit with only that change, then a second commit to add Zmmul?

Yes! haveMulDiv() qualifier has been there. Sure, I'll make two separate commits.

bilalsakhawat · 2021-11-03T18:07:45Z

@scottj97 Actually haveMulDiv() checks were already there. My bad, I added some extra checks. Now I have removed those extra checks.

scottj97 · 2021-11-03T18:13:50Z

It's nice that this is much simplified, but please squash the two commits back into one.

It first appeared that the one commit was doing two things (adding missing haveMulDiv(), and adding Zmmul) which is why I asked for two commits. Since it turns out that was not the case, this PR is only doing one thing (adding Zmmul) so it only needs one commit.

bilalsakhawat · 2021-11-03T18:26:19Z

When it will merge it will squash merge into the master i.e., will go as one commit. Do I need to squash commits anyway ?

scottj97 · 2021-11-03T18:36:15Z

You're assuming the admin who clicks the Merge button chooses the Squash option. I don't think that's a safe assumption.

jrtc27 · 2021-11-03T18:38:00Z

Does M not imply Zmmul, and thus only the latter needs checking?

jrtc27 · 2021-11-03T18:39:26Z

model/riscv_sys_regs.sail

@@ -180,6 +180,8 @@ function haveUsrMode() -> bool = misa.U() == 0b1
 function haveNExt()    -> bool = misa.N() == 0b1
 /* see below for F and D extension tests */

+function haveZmmul()    -> bool = true


As with the bitmanip PR, keep the lists of dummy Z* extension checks sorted, i.e put Zm* below Zk*

Does M not imply Zmmul, and thus only the latter needs checking?

Yes, M does not imply Zmmul. You can read the discussion on this PR

As with the bitmanip PR, keep the lists of dummy Z* extension checks sorted, i.e put Zm* below Zk*

Sure :)

Does M not imply Zmmul, and thus only the latter needs checking?

Yes, M does not imply Zmmul. You can read the discussion on this PR

This contradicts riscv/riscv-isa-manual#648 (comment), which is as close to an authoritative source as I can find. Pointing at someone's own comments on their own Spike pull request is hardly authoritative.

Let's put it in this way. An implementation can have both extensions as a time. When I said does not imply I meant to say If misa.M() ==1 it does not mean that Zmmul extension is there as well. if Either or both of them are in implementation the Mul instructions will work. That's why there is a or between haveMulDiv and haveZmmul.

As far as I understand. The benefit of Zmmul is, as it supports only MUL instructions (Not DIV/REM), it will save hardware in those implementations where DIV/REM instructions are not needed.

One difference between M and Zmmul is that the latter is not controlled by misa.M. If you have Zmmul then the multiply instructions are always enabled. If you have M then the multiply instructions are enabled iff misa.M==1. That's why saying "M implies Zmmul" is inaccurate -- because that would mean that misa.M only controls divide and not multiply.

This Sail model doesn't support modifying misa.M so this is moot here -- but that might change in the future so I think it's better to keep them separate here.

Ah indeed that is an important but subtle difference. I believe modifying misa.M should work if sys_enable_writable_misa is true.

It looks like this code supports modifying bits C/D/F only. And reading it, I see a bug, which I'll file separately.

Oh, right, I skimmed it and thought it was mostly fixing up v, but it's just using v to fix up the existing m.

bilalsakhawat · 2021-11-03T18:47:38Z

You're assuming the admin who clicks the Merge button chooses the Squash option. I don't think that's a safe assumption.

Sure. I'll squash commits.

scottj97 · 2021-11-03T20:15:32Z

@bilalsakhawat-10xe can you please fix the Git commit message. It doesn't make sense to include "Removed extra haveMulDiv() checks" because it doesn't do that anymore.

jrtc27 · 2021-11-04T22:30:11Z

Please amend your commit message to include a verb ("Add support for Zmmul", "Implement Zmmul", etc., don't really care exactly what), it's good practice (otherwise it doesn't actually mean anything as it leaves one asking what are you doing to the zmmul support? adding it? removing it? refactoring it? all possibilities without looking at the changes themselves). Otherwise this looks ready.

jrtc27 · 2021-11-05T18:26:41Z

This was fine until you pushed that merge commit. Never merge into a pull request branch, always rebase the branch. But in this case there was no need to do either, the changes to master don't affect this PR whatsoever.

jrtc27 · 2021-11-05T18:41:26Z

Yes, please rebase in order to lose the merge commit

jrtc27 · 2021-11-05T18:42:07Z

(same goes for your bitmanip, and now scalar crypto, PRs)

bilalsakhawat · 2021-11-05T19:01:33Z

Yes, please rebase in order to lose the merge commit

Steps I have in mind:

revert the merge commit
rebase with master
squash above two into one i.e., rebase with master

Is there a good way to do it?

jrtc27 · 2021-11-05T19:02:29Z

Yes, please rebase in order to lose the merge commit

Steps I have in mind:

revert the merge commit

rebase with master

squash above two into one i.e., rebase with master

Is there a good way to do it?

Just git rebase master, should just work

jrtc27 · 2021-11-05T19:02:52Z

(or git rebase origin/master if your local master branch is behind)

scottj97 · 2021-11-08T22:19:13Z

I wonder if it's a good idea to haveZmmul==true as the default. We also haveMulDiv==true as the default, and it doesn't really make sense to have both of these at once. (As discussed above w.r.t. misa.M.) It seems the configuration system that the Arch Test SIG is working on should have a configuration knob with three choices: None, M, or Zmmul.

bilalsakhawat · 2021-11-11T06:12:35Z

I wonder if it's a good idea to haveZmmul==true as the default. We also haveMulDiv==true as the default, and it doesn't really make sense to have both of these at once. (As discussed above w.r.t. misa.M.) It seems the configuration system that the Arch Test SIG is working on should have a configuration knob with three choices: None, M, or Zmmul.

@jrtc27 you may have a proper answer, why we are using haveZmmul==true as default for now.

bilalsakhawat · 2021-12-04T14:18:47Z

I wonder if it's a good idea to haveZmmul==true as the default. We also haveMulDiv==true as the default, and it doesn't really make sense to have both of these at once. (As discussed above w.r.t. misa.M.) It seems the configuration system that the Arch Test SIG is working on should have a configuration knob with three choices: None, M, or Zmmul.

@jrtc27 you may have a proper answer, why we are using haveZmmul==true as default for now.

@jrtc27 some input from you will be great.

allenjbaum · 2021-12-04T16:16:17Z

I originally defined it such that Zmmul and M were incompatible, but I was told there weren't. Zmmul is redundant with M, but not disallowed. So, stupid, but not forbidden.

jrtc27 · 2021-12-05T05:12:27Z

I originally defined it such that Zmmul and M were incompatible, but I was told there weren't. Zmmul is redundant with M, but not disallowed. So, stupid, but not forbidden.

Ok, thanks for the clarification.

Given we have all the Zb* and Zk* extensions on by default with no way to turn them off (currently), it does seem a bit odd to not provide Zmmul. I know it means setting misa.M to 0 no longer turns off MUL*, but does that actually matter? I don't personally mind either way all that much, just so far the philosophy's been to enable as much as we can by default for maximum utility.

allenjbaum · 2021-12-05T07:25:56Z

From the point of view of architectural tests: the fact that either are on doesn't matter.
If M-extension is unimplemented, we won't ever test DIV/REM ops, so it's irrelevant whether they actually correctly execute those ops
Similarly, if both M-extension and Zmmul are unimplemented, we won't test multiply instructions.

scottj97 · 2021-12-05T17:31:57Z

From the point of view of architectural tests: the fact that either are on doesn't matter.

The misa CSR is part of the architectural spec too. Don't you want to test that it works properly, disabling or enabling instructions as required?

I don't care what the default is for Zmmul in this Sail model.

allenjbaum · 2021-12-06T01:44:18Z

If MISA.M is writable, we could, sort of, but it is a bit pointless. t boot, if M-ext is implemented, MISA.M will be set, and we will test that it works. IF MISA.M is writable, we can test that it can be cleared, and set again, but that is all.
If it is cleared, it is still legal for Mul ops to perform a multiply. On the other hand, if Zmmul and M-ext are both implemented, we could text that Mul ops still work with MISA.M cleared, because Zmmul can't be disabled.
I hadn't thought of that.
In fact, we probably should run all Zmmul tests that way. I need to figure out how to do that without having to re-write all the M tests, since we are just re-using them, running if either M-ext or Zmmul are implemented.

bilalsakhawat · 2021-12-06T05:13:12Z

I know a way how we can run all Zmmul tests without having to re-write all the M tests. We just need to check if Zmmul is in the ISA string and if yes include mul, mulh, mulhsu, mulhu and mulw tests in the database.yaml (it contains the list of tests to be run on Ref and DUT). We just need to change 1-2 files in riscof env.

allenjbaum · 2021-12-06T07:43:03Z

I don't think it's that simple. We potentially have to run the tests twice; once if M-extension is supported (and MISA.M is set, which should be the case after reset. Hmm, our tests probably need to check that) and once if Zmmul is supported and either MISA.M is clear or M-ext isn't supported). I think that means those tests need to be replicated.
I thought we could avoid that, but we can't.

bilalsakhawat · 2021-12-06T07:52:51Z

I don't think it's that simple. We potentially have to run the tests twice; once if M-extension is supported (and MISA.M is set, which should be the case after reset. Hmm, our tests probably need to check that) and once if Zmmul is supported and either MISA.M is clear or M-ext isn't supported). I think that means those tests need to be replicated. I thought we could avoid that, but we can't.

Running test twice is not an issue. We can do that for misa.M and zmmul. The issue will be when collecting coverage, we need to make changes in riscv-ctg as well. Even this is a simple task. But this is not a recommended solution to make changes riscof and riscv-ctg for just zmmul. Replicating the tests is the best solution.

allenjbaum · 2021-12-06T19:24:50Z

I talked to Krste. He is suggesting that if an implemention does have both Zmmul AND M-extension AND misa.M is RW, then vendor is responsible for running the tests twice with different boot code, the normal one and one that clears misa.M before running tests. It’s unlikely that anyone will ever build this specific configuration (why would you ever disable M?) so making the umplementir do the extra work is fine.

…

On Sunday, December 5, 2021, Bilal Sakhawat ***@***.***> wrote: I don't think it's that simple. We potentially have to run the tests twice; once if M-extension is supported (and MISA.M is set, which should be the case after reset. Hmm, our tests probably need to check that) and once if Zmmul is supported and either MISA.M is clear or M-ext isn't supported). I think that means those tests need to be replicated. I thought we could avoid that, but we can't. Running test twice is not an issue. We can do that for misa.M and zmmul. The issue will be when collecting coverage, we need to make changes in riscv-ctg as well. Even this is a simple task. But this is not a recommended solution to make changes riscof and riscv-ctg for just zmmul. Replicating the tests is the best solution. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#122 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJTDNKULGBWD42YN4DDUPRTV5ANCNFSM5HI5LHDA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

allenjbaum · 2021-12-08T18:40:39Z

I think I've discussed this elsewhere, but the corner case only occurs when there is an implementation that has both M-extension and Zmmul, AND misa.M is read-write, so the M-extension can be disabled (which effectively disables only DIV/REM ops, since Zmmul can't be disabled).
This is an unlikely enough to be implemented that instead of burdening our test infrastructure with handling this, we will will require the vendor to run tests twice: once normally and once with boot code augmented to clear misa.M. The latter only needs to execute M-extension tests. Both m-extension tests reports should be submitted.

bilalsakhawat · 2021-12-08T19:17:58Z

So I think, the changes I made in this PR are good enough for now. Later we may add a plusargs in sail '--isa=RVxxx' like we have in spike. Then for 64-bit we can have the cases like:

RV64IM (misa.M == true, Zmmul == false)
RV64IM_Zmmul (misa.M == true, Zmmul == true)
RV64I_Zmmul (misa.M == false, Zmmul == true)
And a case where we have neither for them enable (misa.M == false, Zmmul == false)
depending on the plusargs '--isa' string.

jrtc27 · 2021-12-08T19:20:05Z

Well, it's not a plusarg, those are specifically +foo and are so called due to the leading +, and are pretty specific to the hardware space (and thus spike's fesvr/htif/whatever stuff..). But yes, eventually all the Zfoo should be dynamically configurable via a command-line argument, ideally as an arch string that matches what toolchains use.

jrtc27 · 2021-12-08T19:21:33Z

So, if nobody has any strong opinions on this, I suggest we go ahead and merge it with Zmmul defaulting to enabled, given so far we've only disabled extensions by default if they conflict?

bilalsakhawat · 2021-12-08T19:45:55Z

Well, it's not a plusarg, those are specifically +foo and are so called due to the leading +, and are pretty specific to the hardware space (and thus spike's fesvr/htif/whatever stuff..). But yes, eventually all the Zfoo should be dynamically configurable via a command-line argument, ideally as an arch string that matches what toolchains use.

Thanks for correction. I choose the wrong terms actually, I should have said command-line argument.

allenjbaum · 2021-12-14T16:29:17Z

From the point of view of ACTs - it is our intention that we don't execute tests for extensions that don't have support (or are disabled) because results are unpredictable. At reset, all extensions that implemented are enabled, regardless of whether it is possible to disable them. So, defaulting them to enabled seems just fine.

martinberger · 2022-01-18T11:13:06Z

@scottj97 @jrtc27 @allenjbaum @bilalsakhawat-10xe I would like to merge this PR this week. Any objections to that?

allenjbaum · 2022-01-18T18:46:27Z

not from me!

…

On Tue, Jan 18, 2022 at 3:13 AM Martin Berger ***@***.***> wrote: @scottj97 <https://github.com/scottj97> @jrtc27 <https://github.com/jrtc27> @allenjbaum <https://github.com/allenjbaum> @bilalsakhawat-10xe <https://github.com/bilalsakhawat-10xe> I would like to merge this PR this week. Any objections to that? — Reply to this email directly, view it on GitHub <#122 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHPXVJQBEYJG3JA3ABCJ4YDUWVDM5ANCNFSM5HI5LHDA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

jrtc27 · 2022-01-18T18:50:39Z

Nope, long overdue

martinberger · 2022-01-19T19:15:01Z

Merged.

Thanks @bilalsakhawat-10xe @scottj97 @jrtc27 @allenjbaum

jrtc27 reviewed Nov 3, 2021

View reviewed changes

bilalsakhawat force-pushed the zmmul_dev branch from 22af6e5 to 7c334f3 Compare November 3, 2021 20:06

bilalsakhawat force-pushed the zmmul_dev branch from 7c334f3 to 97757e6 Compare November 4, 2021 04:38

bilalsakhawat force-pushed the zmmul_dev branch from 97757e6 to fe9e82a Compare November 5, 2021 01:18

bilalsakhawat changed the title ~~Support for Zmmul~~ Add Support for Zmmul Nov 5, 2021

Add support for Zmmul

4cb28ae

bilalsakhawat force-pushed the zmmul_dev branch from 1318b81 to 4cb28ae Compare November 5, 2021 19:52

scottj97 approved these changes Jan 19, 2022

View reviewed changes

martinberger approved these changes Jan 19, 2022

View reviewed changes

martinberger merged commit 56125e6 into riscv:master Jan 19, 2022

bilalsakhawat added a commit to bilalsakhawat/sail-riscv that referenced this pull request Jan 20, 2022

Add support for Zmmul (riscv#122)

807129c

scottj97 mentioned this pull request Jul 14, 2022

Is Zmmul a subset of M? riscv/riscv-isa-manual#869

Closed

Add Support for Zmmul #122

Add Support for Zmmul #122

Conversation

bilalsakhawat commented Nov 3, 2021

scottj97 commented Nov 3, 2021

bilalsakhawat commented Nov 3, 2021

bilalsakhawat commented Nov 3, 2021

scottj97 commented Nov 3, 2021 • edited Loading

bilalsakhawat commented Nov 3, 2021 • edited Loading

scottj97 commented Nov 3, 2021

jrtc27 commented Nov 3, 2021

Choose a reason for hiding this comment

bilalsakhawat Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bilalsakhawat commented Nov 3, 2021

scottj97 commented Nov 3, 2021

jrtc27 commented Nov 4, 2021

jrtc27 commented Nov 5, 2021

jrtc27 commented Nov 5, 2021

jrtc27 commented Nov 5, 2021

bilalsakhawat commented Nov 5, 2021

jrtc27 commented Nov 5, 2021

jrtc27 commented Nov 5, 2021

scottj97 commented Nov 8, 2021

bilalsakhawat commented Nov 11, 2021

bilalsakhawat commented Dec 4, 2021 • edited Loading

allenjbaum commented Dec 4, 2021

jrtc27 commented Dec 5, 2021

allenjbaum commented Dec 5, 2021

scottj97 commented Dec 5, 2021

allenjbaum commented Dec 6, 2021

bilalsakhawat commented Dec 6, 2021 • edited Loading

allenjbaum commented Dec 6, 2021

bilalsakhawat commented Dec 6, 2021

allenjbaum commented Dec 6, 2021 via email

allenjbaum commented Dec 8, 2021

bilalsakhawat commented Dec 8, 2021

jrtc27 commented Dec 8, 2021

jrtc27 commented Dec 8, 2021

bilalsakhawat commented Dec 8, 2021

allenjbaum commented Dec 14, 2021

martinberger commented Jan 18, 2022

allenjbaum commented Jan 18, 2022 via email

jrtc27 commented Jan 18, 2022

martinberger commented Jan 19, 2022

scottj97 commented Nov 3, 2021 •

edited

Loading

bilalsakhawat commented Nov 3, 2021 •

edited

Loading

bilalsakhawat Nov 3, 2021 •

edited

Loading

jrtc27 Nov 3, 2021 •

edited

Loading

bilalsakhawat commented Dec 4, 2021 •

edited

Loading

bilalsakhawat commented Dec 6, 2021 •

edited

Loading