Skip to content
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

M implies Zmmul #1121

Merged
merged 2 commits into from
Sep 20, 2023
Merged

M implies Zmmul #1121

merged 2 commits into from
Sep 20, 2023

Conversation

nick-knight
Copy link
Contributor

@nick-knight nick-knight commented Sep 11, 2023

Zmmul is defined to be a subset of M. It follows that M implies Zmmul. This closes #869. It also resolves a question raised in the Toolchains SIG: riscv-non-isa/riscv-asm-manual#91 (comment).

This PR adds this implication to a table in the "ISA Naming Conventions" chapter. (EDIT: and also in the M chapter.)

@aswaterman
Copy link
Member

Can we also clean up the text in the Zmmul section in the M chapter to make the implication clear?

@nick-knight
Copy link
Contributor Author

I welcome feedback on how to better clean up the M chapter. I just added a sentence.

@topperc
Copy link
Collaborator

topperc commented Sep 11, 2023

This potentially breaks using a new clang with an old gnu assembler if the old assembler doesn't know about zmmul. That's why I was hesitant to make the implication in the tools.

@nick-knight
Copy link
Contributor Author

@topperc will this same problem arise in the future when we define a subset of an existing standard extension, making the superset imply the subset? (This seems unfortunate.)

@aswaterman
Copy link
Member

I don't want to break the new compiler + old assembler combo (unless perhaps we are checking at compiler build/install time that it's compatible with the installed assembler).

But maybe this is an indication that it's inadvisable to retcon implication relationships. I'm really not sure; I want to do whatever's most pragmatic.

@nick-knight
Copy link
Contributor Author

In the issue associated with this PR, #869, @kasanovic stated:

Zmmul is implied by M - it is a proper subset of the instructions in M.

If there is debate about this, perhaps we should move the discussion back to that issue.

@allenjbaum
Copy link

allenjbaum commented Sep 14, 2023 via email

@nick-knight
Copy link
Contributor Author

nick-knight commented Sep 14, 2023

Hi Allen, are you reiterating the comment you made 14 months ago over at #869 ? If so, do you have a response to Krste's response to you?

Taking a step back, I was filing this PR to clean up what I perceived as technical debt. Clearly there is more debate to be had over at #869 . I didn't mean to fragment the discussion.

@allenjbaum
Copy link

allenjbaum commented Sep 14, 2023 via email

@kito-cheng
Copy link
Member

To me, it's part of cannibalization rule we should define clearly, what's the standard rule for those situation? we should have consistent rule.

The question is something like does rv32im_zmmul is equal to rv32im? The answer is yes I believe, but which one is canonicalization form? IMO human and machine can easier to know two arch string are equivalent after canonicalization , but the rule is broken.

It's not human should care in general, but psABI spec do care about this since psABI specify the arch string should canonicalize the arch string before encoding into file, so NO clear canonicalization rule made psABI spec ambiguous.

C and Zca are the same issue IMO.

That's affect toolchain AND how people check a extension, what if user want to check multiplication instructions? user need to check two extension, __riscv_zmmul or __riscv_m rather than just check __riscv_zmmul.

And again, same situation for Zca vs C.

Also the situation is happened between V and Zve* extensions, and spec explicitly say V require/depend on Zve*, why we consider inconsistent behavior for M and Zmmul?

So I strongly support M should implied Zmmul AND C should implied Zca.

@topperc
Copy link
Collaborator

topperc commented Sep 15, 2023

My primary concerns with making M imply Zmmul is that LLVM passes all implied extensions through to ELF attributes and probably on to external tools like assembler. Should we canonicalize the string passed on to remove extensions implied by other extensions?

My other concern is that telling users they can check __riscv_zmmul to detect multiply instructions means their code will only detect multiply instructions when used with a newer compiler that knows about Zmmul and believes Zmmul to be a subset of M. I thought I could suggest undeprecating __riscv_mul but gcc doesn't set it for Zmmul, clang does.

Not compiler related, but if hwprobe supported Zmmul would it return true for a CPU that supports M? Or would that require the DTS to say Zmmul in addition to M? I don't if hwprobe also has to know about implications.

@nick-knight
Copy link
Contributor Author

I think the architects need to reach a consensus on the definition of "implies", "subset", etc., particularly in the presence of privileged state like misa, mstatus, etc. While this thread concerns M vs. Zmmul, I expect this debate to flare up elsewhere (V vs. Zve*, C vs. Zc*, ...). This is a broader debate than I set out to address in this PR.

Compile-time feature detection obviously cannot handle the possibility of misa, so I think that toolchain folks can ignore this debate, and focus on building things that are useful to software engineers. I think this discussion belongs over at the riscv-non-isa repos.

@allenjbaum
Copy link

allenjbaum commented Sep 15, 2023 via email

@kasanovic
Copy link
Collaborator

There has always been clear agreement on what implies/requires means (it means the features within the implied extension must exist if the implying extension is present). M_Zmmul should cause no difference in behavior than saying M on command line, and M is the canonical representation of the union. The issue is not the definition of implies, the issue is with how toolchains can manage the extension name space.

The toolchain problem is that life is simpler if they only have to represent the finest granularity of extension optionality, and not worry that existing extensions will be broken into sub extensions. OTOH, maintaining a feature vector at the granularity of every indivisible ISA feature (e.g., not only a field in a CSR, but an individual settable value of a field in a CSR) is not practical.

To answer @topperc question, all implied extensions should be made visible in any feature vector of "extensions supported" visible to downstream software. When a new sub extension is added, this will add a new feature in the feature vector. old software would only be checking for the old larger extension name (M here), and new software that only cares about the subextension will only check for the new sub-extension flag and should operate correctly if only the larger extension name is present. I don't see how any other behavior would make sense?

The issue with misa.M definition is separate from toolchain command-line option parsing. M/Zmmul specs do not mention misa, so should not read on misa spec. misa is not visible to almost all code ( I know tests might look there, but even nearly all of those shouldn't). misa spec requires that if M is set, then M is available. If only Zmmul is present misa.M should be clear. If misa.M is writable and M is cleared, the specs say that the hart behaves as if M not present, where behavior includes the opcodes being "reserved". Whether Zmmul is still usable when misa.M is clear is implementation-defined; misa allows the behaviors to be "reserved" in this case - so Zmmul if still available, it acts as a standard extension. This I believe is the only way to have a consistent view of misa behavior matching the ratified specifications. But the real problem is that misa was not designed to support fine-grain extensions, and dynamic switching was also not architected to support all use cases.

@topperc
Copy link
Collaborator

topperc commented Sep 15, 2023

To answer @topperc question, all implied extensions should be made visible in any feature vector of "extensions supported" visible to downstream software. When a new sub extension is added, this will add a new feature in the feature vector. old software would only be checking for the old larger extension name (M here), and new software that only cares about the subextension will only check for the new sub-extension flag and should operate correctly if only the larger extension name is present. I don't see how any other behavior would make sense?

assembers and compilers reject a -march command line option with unknown extension names. When gcc (or clang in no integrated assembler mode) invoke the assembler they need to pass a -march to the assembler. Since gcc and clang can be upgraded independent of the assembler this can cause an error if the compiler knows M implies Zmmul, but the assembler has never heard of Zmmul.

Maybe you're suggesting the parsing of -march in tools should ignore unknown extension names?

@allenjbaum
Copy link

allenjbaum commented Sep 15, 2023 via email

@nick-knight
Copy link
Contributor Author

I am satisfied with @kasanovic's argument that this PR is compatible with misa behavior, so I'm reopening it.

My perspective remains that this is simply a (non-normative) clarification. If there are ramifications for certain toolchains, that discussion belongs elsewhere.

@nick-knight nick-knight reopened this Sep 16, 2023
@a4lg
Copy link
Contributor

a4lg commented Sep 17, 2023

@kito-cheng
I was a bit hesitant to make CZca implication without clear resolution (so asked everywhere for clarification including riscv-non-isa/riscv-asm-manual#91) because this is far more destructive than MZmmul but generally, I think feature == extension is simpler (while I feel that recent decision on the Toolchain SIG ― extension implication should only apply when the specification specifies ― makes sense).

@topperc
I admit that some version compatibility problem (between the compiler and the assembler) exists but I don't think this is a major blocker for this issue because it's hard to imagine that both programs are out of sync so much.

@topperc
Copy link
Collaborator

topperc commented Sep 17, 2023

@topperc I admit that some version compatibility problem (between the compiler and the assembler) exists but I don't think this is a major blocker for this issue because it's hard to imagine that both programs are out of sync so much.

That's probably true now that Zmmul has been in tools for a while, but it wasn't true when Zmmul was first added to a compiler. The installed assembler could very easily not have known about it.

We could probably change it in the tools now without much issue. But does that mean for every future subset extension we need some grace period to avoid creating compiler/assembler incompatibility?

I agree this isn't a blocker for this issue.

@kasanovic
Copy link
Collaborator

kasanovic commented Sep 17, 2023

Let me clarify my comment on feature vector. The feature vector is an abstract notion of what exists in the ISA set in some environment. How this is presented to various parts of older software will vary, but if a given piece of software doesn't understand a subset extension there's nothing really to do except update it or force all surrounding software to adapt.

For the specific case that some toolchain components don't yet understand a new subset extension, the components can only continue to use the larger extensions that they do understand. This will mean that a compiler that understands Zmmul should set Zmmul, not M, on assembler command line if only Zmmul is specified on its command line, and the assembler will rightly complain if it doesn't understand Zmmul. If user specified both Zmmul and M, then the canonical expansion will simplify to M, and everything should work. Another workaround when Zmmul is actually intended but not supported is for the user to use M and not Zmmul. Yes, this will incorrectly cause assembler to not warn about non-existent divide instructions but that's the best the toolchain can support. I don't see a simpler alternative path here - the components have to understand the subset extension to use it precisely. The assembler simply needs updating to understand Zmmul. (I'll note from almost all users' perspective, if the assembler in a toolchain doesn't support Zmmul, the compiler doesn't either.)

@aswaterman aswaterman merged commit 528e4e6 into riscv:main Sep 20, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is Zmmul a subset of M?
7 participants