-
Notifications
You must be signed in to change notification settings - Fork 600
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
Add Zmmul section to M chapter #648
Conversation
Commentary is incorrect that it cannot co-exist with the M extension, e.g. F can coexist with D. It would be non-canonical (and redundant) but legal to have Zmmul_M or M_Zmmul in ISA string. I would also drop the commentary around exception and non-conforming use of {M-Zmmul} opcodes - this is common across every instruction. Zmmul should be written as adding to base, not as subtracting from M. |
I thought you might say both of those things, but wanted to maintain fidelity to Allen's original text before getting feedback from others. |
I will disagree with Krste's example of F and D coexisting. D requires F.
Zmmul does not require M, nor does M require Zmmul, so it is an entirely
different case.
The problem with allowing both is that Zmmul will apply the -mno-div flag
to the compiler.
The toolchain will thus think that there is no hardware divide - but
M-extension tells it that it does, and that makes the toolchain ..
confused. OR, at least toolchain implementers.
It is not merely redundant, there are side effects.
But if you want to allow that - it's on your head. I wanted to be very
specific about this in order to avoid any toolchain issues, and to make
this as clean as possible.
And there is no good reason to allow it, frankly - it adds nothing and
costs nothing to disallow it.
The changes I've added to riscv-config will flag this as an error, but I
can remove that check. Be careful what you wish for.
…On Mon, May 17, 2021 at 1:41 AM Krste Asanovic ***@***.***> wrote:
Merged #648 <#648> into
master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#648 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJUAPSY26FXHS7G5VWTTODJDHANCNFSM446KNVMA>
.
|
Does here version for |
M does imply/require Zmmul. ISA strings are purely additive, and tool chains should not assume absence of a feature because of the presence of a different feature. F and D are a fine example of this. Just because string includes F does not imply it does not include D. The gcc -mno-div flag should continue to be used to exclude generate of divide instructions, but also, lack of M (and lack of any other future ISA string that might include divide) should also tell compiler not to use instructions that are not present in ISA. |
D requires F. I don't think that means that M requires Zmmul - because
there isn't a Zmmul now and M seems to muddle along just fine anyway.
If GCC and LLVM had an architecture flag -mmul-only rather than an mno-div,
I'd find this argument a bit easier to accept.
But: clearly the toolchain thinks about these differently than you are
thinking about it, and that concerns me.
The way I'm thinking about it is that Zmmul doesn't remove anything. IF you
allowed them both M and Zmmul, then it might.
By instead saying they shouldn't ever both appear in the arch string, I'm
avoiding the "removal" issue. It doesn't come up, and doesn't need to.
If the toolchain doesn't barf on this (I won't pretend to know the answer)
it means that an implementation that meets a platform profile that requires
M - and expects a performant divide operation - won't get it.
If that's OK (or my assumption above are incorrect, e.g. -mno-div is
ignored), I'm fine with this.
A possible compromise is non-normative text that describes the side effects
of specifying both perhaps?
…On Mon, May 17, 2021 at 9:26 AM Krste Asanovic ***@***.***> wrote:
M does imply/require Zmmul.
ISA strings are purely additive, and tool chains should not assume absence
of a feature because of the presence of a different feature.
F and D are a fine example of this. Just because string includes F does
not imply it does not include D.
The gcc -mno-div flag should continue to be used to exclude generate of
divide instructions, but also, lack of M (and lack of any other future ISA
string that might include divide) should also tell compiler not to use
instructions that are not present in ISA.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#648 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJXETUE3CKVYQDHXQ6LTOE7TJANCNFSM446KNVMA>
.
|
The We have no choice to support the legacy compiler flag for now. But we can avoid adding any more subtractive flags going forward, and we can deprecate this one over time. So it need not affect our long-term thinking about ISA extensions. I think the compiler documentation is the right place to describe this idiosyncrasy, not the spec. |
Yes, the tool chain has to accept the new ISA extension and "do the right thing". I think -mno-div is OK as a compiler option, but it's meaning is for the compiler not to generate divide instructions, not to indicate what the ISA is. So for example, -mno-div should not set ELF flags in binary to indicate ISA doesn't have divide - it should simply not generate divide instructions. |
From the view of compiler developer, I strongly prefer using We have ELF attribute to record arch string already, so we can know the which extension are used for the binary. |
I was basing the D.O.D status on the fact that -mno-div was already
implemented and no further work needed to be done.
If we need to get Zmmul into the toolchains before we can proceed how do
we/I proceed?
I have no one lined up to do that work, and probably wouldn't have started
the process if it needed more than -mno-div because of htat.
…On Mon, May 17, 2021 at 6:26 PM Kito Cheng ***@***.***> wrote:
From the view of compiler developer, I strongly prefer using -march as
the unify interface rather than add options like -m[no]-div, -m[no]-mul
and potential option in future.
We have ELF attribute to record arch string already, so we can know the
which extension are used for the binary.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#648 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJSKMETNC5436PAE5NTTOG647ANCNFSM446KNVMA>
.
|
Sounds like the tail wagging the dog. Ask for a waiver, and attach Kito’s most recent comment to justify the waiver request? |
I think here is 2 options for GCC*:
Personally I would prefer 1, I could implement that on upstream GCC and ask LLVM community to implement that once this settle down.
|
There are university projects that use -mno-div. Removing that before adding zmmul will cause problems for those users. Certainly once zmmul support exists, we don't need -mno-div anymore, and can deprecate it. Probably best to have a transition period though or we will have some confused users. Also probably best that it gets formally ratified before -mno-div is removed or we will have to explain to confused users how to use explicit version numbers to enable a non-ratified feature, and what exactly those valid version numbers are. Though the doc doesn't give zmmul any explicit version number, so not obvious what version numbers we should use for this non ratified feature. GCC still has the rule that an extension must be frozen before we can add support for it on mainline, and I haven't seen anything about zmmul being frozen. I don't even see it in the specification status spreadsheet. Maybe the addition of this section to the doc implies that it is frozen? |
I agree with Jim's take. I don't believe Zmmul has been declared frozen. Adding it to a draft version of a spec is not an implicit indication of frozennesss (even though we ordinarily only merge stuff that we consider very close). I will assign Zmmul a version number, 0.1, for now. (My expectation is that it will be frozen as-is, and the version number will soon be increased to 1.0 without any changes being made.) |
Oh, I have no illusions that it is frozen - though I would say that this is
the big sticking point.
I overlooked the spec status spreadsheet, and was just filling things in on
the Fastrack Definition of Done spreadsheet - so I've just added it to the
status there.
I don't think we need something put into mainline GCC now; a branch with
that support would be good enough.
But, until the branch exists (and a similar LLVM branch; I had asked
someone to do add -mno-div support, to LLVM but that needs to be re-thought
out given Jim's comments), we need a waiver
From a D.O.D standpoint, then, the only things left to get to freeze are
- getting all the HCs to review and approve it (along with a waiver on
the toolchain support)
- Modify the riscv-config PR that flags M-extension and Zmmul coexisting
(I thought the canonical arch string rules forbade it, but Krste indicated
it is redundant but OK)
- an ACT PR that replicates the mul tests into a Zmmul directory and uses
-mno-div as the compiler flag (this will get removed when v3 of the
framework is merged)
…On Tue, May 18, 2021 at 9:29 PM Andrew Waterman ***@***.***> wrote:
I agree with Jim's take.
I don't believe Zmmul has been declared frozen. Adding it to a draft
version of a spec is not an implicit indication of frozennesss (even though
we ordinarily only merge stuff that we consider very close).
I will assign Zmmul a version number, 0.1, for now. (My expectation is
that it will be frozen as-is, and the version number will be increased to
1.0 without any changes being made.)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#648 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJX6FX77YKFTSNWCLMLTOM5BVANCNFSM446KNVMA>
.
|
Update from LLVM community[1]: Summary: [1] https://reviews.llvm.org/D102839 Note: For GNU toolchain one cycle means at least one year, deprecate is one cycle, obsolete in next cycle, so removing the |
thanks very very!
…On Thu, May 27, 2021 at 11:11 PM Kito Cheng ***@***.***> wrote:
Update from LLVM community[1]:
Summary:
They intend to support both -mno-div and zmmul extension, but -mno-div
will be deprecated once zmmul is frozen.
[1] https://reviews.llvm.org/D102839
Note:
Deprecating not means we will remove the functionality of -mno-div
immediately, we will emit warning but still functional, then obsolete that
in next release cycle.
For GNU toolchain one cycle means at least one year, deprecate is one
cycle, obsolete in next cycle, so removing the -mno-div should be at
least 2 years later after zmmul extension frozen.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#648 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHPXVJU6CWRFR62J3L32I43TP4XZBANCNFSM446KNVMA>
.
|
let's try that again - thanks very much!
On Fri, Jun 4, 2021 at 1:21 PM Allen Baum ***@***.***>
wrote:
… thanks very very!
On Thu, May 27, 2021 at 11:11 PM Kito Cheng ***@***.***>
wrote:
> Update from LLVM community[1]:
>
> Summary:
> They intend to support both -mno-div and zmmul extension, but -mno-div
> will be deprecated once zmmul is frozen.
>
> [1] https://reviews.llvm.org/D102839
>
> Note:
> Deprecating not means we will remove the functionality of -mno-div
> immediately, we will emit warning but still functional, then obsolete that
> in next release cycle.
>
> For GNU toolchain one cycle means at least one year, deprecate is one
> cycle, obsolete in next cycle, so removing the -mno-div should be at
> least 2 years later after zmmul extension frozen.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#648 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AHPXVJU6CWRFR62J3L32I43TP4XZBANCNFSM446KNVMA>
> .
>
|
This patch implements recently merged extension [Zmmul](riscv/riscv-isa-manual#648), a subextension of M (Integer Multiplication and Division) consisting only multiplication part of it. Differential Revision: https://reviews.llvm.org/D103313
I had added |
I can't review your changes, but I can send you the directions to test it. I don't have your contact information though. |
thanks |
Contributed by @allenjbaum, with some of my own edits.