[RISCV] Support '.option arch' directive

[StephenFan]

Summary

The proposal of '.option arch' directive is https://github.com/riscv-non-isa/riscv-asm-manual/pull/67

Note: For '.option arch, +/-' directive, version number is not yet supported.

[StephenFan]

Address comments.

[jrtc27]

What happened to your new test?

[jrtc27]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2732

These errors all need their grammar fixing

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

37

This is a *weird* interface

148

You're still putting quotes around it

llvm/test/MC/RISCV/option-invalid.s

17

Stop using quotes except when specifically testing that quotes get rejected

[hiraditya]

This patch is needed for zbb support as per: https://lore.kernel.org/all/20230117122447.y6tdsmsxqdwf76ri@orel/

[jrtc27]

In D123515#4189301, @hiraditya wrote:

This patch is needed for zbb support as per: https://lore.kernel.org/all/20230117122447.y6tdsmsxqdwf76ri@orel/

For Zbb support *in the Linux kernel for its hand-written assembly routines*. Zbb works in Clang/LLVM today, and you can get it with -march=...zbb..., whether for compiler-generated code or hand-written assembly.

[hiraditya]

ping @StephenFan

[StephenFan]

Address comments and add back test file.

[StephenFan]

Grammar fixing.

[luismarques]

llvm/test/MC/RISCV/option-arch.s

65

Nit: separated

109

You should also test that something like +d, -d enables the dependent extensions (f).

[luismarques]

Overall LGTM. Only some minor points left to address.

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2711

Nit: unneeded empty line space.

[luismarques]

llvm/test/MC/RISCV/option-invalid.s

31–32

The error should be that we don't (yet) support version numbers here, not that "extensions don't rely on version numbers", since the spec for .option arch does include explicit support for version numbers. Or just add such support to this patch, instead of being a follow-up patch.

[asb]

First and foremost, thank you for keeping this up to date and iterating upon it while the .option arch proposal was under review in the asm manual. I think we're basically almost there.

I've left a few inline comments (and +1 on Luis' suggestion to cover the case where you e.g. +d then -d, and +f should remain enabled.)

What is your intended behaviour for experimental extensions? It looks like they would be accepted with a fullarch string including the full version, but it seems are rejected for for +foo/-foo (when invoking llvm-mc directly at least). I think that is fine, as we don't really seek to make experimental extensions fully first class citizens. But it seems as though this implementation rejects version numbers in for +foo and -foo while the spec accepts a version such as 2p1 or 2. Accepting a version would give an easy way of supporting experimental extensions too. We should really have some test coverage for experimental extensions.

I _think_ we've probably resolved our discussions on versioning and extensions enough that we shouldn't have too much uncertainty on how this is handled. But once the review comments are addressed, I'd be open to landing this without the version parsing support if you don't have time to look at that - if so, I'd suggest changing the error from "Don't specify version number, extensions don't rely on version numbers" to "Extension version number parsing not currently implemented" or similar.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

144

Maybe emitDirectiveOptionArchFullArch or similar? As = isn't part of the grammar in the committed version of the spec.

llvm/test/MC/RISCV/option-invalid.s

17

It would be good to check .option arch rv32ifd, +f, +d or similar (i.e. an input with a fullarch string coming first).

[luismarques]

Adding a test showing that using .option arch in inline assembly doesn't affect ELF attributes nor set the EF_RISCV_RVC ELF flag would be nice. The latter can also be tested for assembly files.

[MaskRay]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2728

unneeded line wrapping? Did you use git clang-format on the modified lines?

llvm/test/MC/RISCV/option-arch.s

94

here and throughout, add a space in #CHECK:

[StephenFan]

Address comments.

[luismarques]

Thanks for all of the improvements so far.

Two additional problems I noticed:

1. It currently accepts non-extension features like .option arch, +svnapot, +relax.

2. This patch currently allows switching between rv32 and rv64 when using the full arch string. The spec doesn't seem to say anything about that and I could see some obscure uses for it, but I'm not sure we actually want to support it, so it's probably safer to reject it (opinions from others about this are welcome). Either way, we should have test coverage for that scenario.

llvm/test/MC/RISCV/option-invalid.s

34–39

Nit: don't capitalize the error message, for consistency.

[hiraditya]

ping: @StephenFan

[StephenFan]

Reject +relax

[StephenFan]

Reject switching between rv32 and rv64

[StephenFan]

Reject switching between rv32 and rv64

[StephenFan]

In D123515#4248222, @luismarques wrote:

Thanks for all of the improvements so far.

Two additional problems I noticed:

1. It currently accepts non-extension features like .option arch, +svnapot, +relax.

svnapot can be parsed from arch string. I think we need an API in RISCVISAInfo to decide that some extensions are non-extension features, only rejecting in the logic of parse .option arch directive seems tricky.

[jrtc27]

There's nothing wrong with .option arch, +svnapot; it's an extension, so it's valid to put in the arch string, it's just a bit pointless as it doesn't actually do anything to the toolchain.

[craig.topper]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2758

Extensions can have numbers in them that aren't versions. For example "zvl32b". Digits are only version numbers if they are the end of the string.

2777

"It is invalid to disable an extension if there are other enabled extensions that depend on it."

2780

I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.

This also doesn't cover cases that can't be expressed in the .td file. At one point Zve32f required either F or Zfinx. But F and Zfinx are mutually exclusive so we could't have Zve32f imply either of them in the .td file. All we could do is check it in RISCVISAInfo::checkDependency.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

150

Can emitDirectiveOptionArchPlus/emitDirectiveOptionArchMinus be combined by passing a bool for + or -?

[hiraditya]

ping: @StephenFan

[StephenFan]

Looks like some users are looking forward to this patch, would you mind accepting it first and then addressing your comments separately @craig.topper ?

[craig.topper]

In D123515#4351899, @StephenFan wrote:

Looks like some users are looking forward to this patch, would you mind accepting it first and then addressing your comments separately @craig.topper ?

Sure. LGTM

[jrtc27]

Issues need fixing before it lands, making exceptions sets a bad precedent

llvm/test/MC/RISCV/option-arch.s

114

This seems like a holdover from back when this was allowing quoted strings, and doesn't distinguish it from the test earlier in the file

[StephenFan]

Accept extension name that has digits.

[StephenFan]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2780

I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.

Can this be handled by RISCVISAInfo? When I parse RISCVIAInfo from featureBits, it always updates implication.

[kito-cheng]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2780

Yeah, use RISCVISAInfo should be reasonable, no reason to duplicate and maintain the logic here :)

An idea thought here:

• Convert feature bits to feature string vector
• Convert extension list to a vector<string>
• Implement a new parse function static llvm::Expected< std::unique_ptr< RISCVISAInfo > > RISCVISAInfo::parseArchOption (const std::vector< StringRef > &BaseFeatures, const std::vector< StringRef > &ExtensionList), then handle all ISA stuff logic here.

[StephenFan]

Add test that check impliest transtively

[StephenFan]

Add TODO.

[StephenFan]

Check dependency when enabling an extension.

[StephenFan]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2780

I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.

Thanks, @craig.topper and @kito-cheng . But it does check for transitivity here. And I added a test of this:

.option arch, +d, -zicsr

llvm-mc can report an error correctly. So there is no functional mistake and if we want to use RISCVISAInfo, we just need a NFC patch.

This also doesn't cover cases that can't be expressed in the .td file. At one point Zve32f required either F or Zfinx. But F and Zfinx are mutually exclusive so we could't have Zve32f imply either of them in the .td file. All we could do is check it in RISCVISAInfo::checkDependency.

Do you mean like that we can't enable the zfinx extension if f extension exists? If so, I have addressed it.

[craig.topper]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2780

I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.

Thanks, @craig.topper and @kito-cheng . But it does check for transitivity here. And I added a test of this:

.option arch, +d, -zicsr

llvm-mc can report an error correctly. So there is no functional mistake and if we want to use RISCVISAInfo, we just need a NFC patch.

I guess it works because F was implied by D so the check was applied directly against F when Zicsr was disabled.

This also doesn't cover cases that can't be expressed in the .td file. At one point Zve32f required either F or Zfinx. But F and Zfinx are mutually exclusive so we could't have Zve32f imply either of them in the .td file. All we could do is check it in RISCVISAInfo::checkDependency.

Do you mean like that we can't enable the zfinx extension if f extension exists? If so, I have addressed it.

I wasn't specifically referring to that. I was only saying that the implies list in the .td file can be incomplete.

[craig.topper]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2780

The .td file for Zve32f requiring F was only updated recently here https://reviews.llvm.org/D150021

[StephenFan]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2780

I don't think the implies list is normalized to include transitive implications. For example if A implies B and B implies C. I don't think the C is A's implies list.

Thanks, @craig.topper and @kito-cheng . But it does check for transitivity here. And I added a test of this:

.option arch, +d, -zicsr

llvm-mc can report an error correctly. So there is no functional mistake and if we want to use RISCVISAInfo, we just need a NFC patch.

I guess it works because F was implied by D so the check was applied directly against F when Zicsr was disabled.

Yeah, it works because F is implied when D is enabled. And when we were checking zicsr, F's imply list was checked. So llvm-mc would report an error.
Maybe I have not got your point about " the implies list in the .td file can be incomplete.". Would you mind giving me an assembly example to show my mistake?

[craig.topper]

llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp

2780

I don't have an example right now. All I'm trying to say is that the Implies field in FeatureKV has been incorrect in the past. 3 weeks ago it did not say that Zve32f implies F. So I think your patch would have allowed F to be disabled when Zve32f was enabled. This is no longer an issue, but I can't guarantee it won't be an issue again in the future.

[craig.topper]

@jrtc27 are there any remaining issues you'd like to see addressed in the first commit. Android developers are really anxious to get this in to enable V extension support in the kernel.

[jrtc27]

In D123515#4370386, @craig.topper wrote:

@jrtc27 are there any remaining issues you'd like to see addressed in the first commit. Android developers are really anxious to get this in to enable V extension support in the kernel.

I still really don't like the target streamer interface... using a bool for +/- would at least help, but IMO this should surely be something like an emitOptionArch that takes a list of string-enum pairs, where the enum is something like Full/Plus/Minus. It's the PrefixEmitted that feels particularly egregious.

llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp

150

Is clang-format really happy with the lack of blank lines?

[asb]

Am I right in thinking we're at the point now where we're discussing slightly cleaner implementation approaches but are happy the functionality is correct and faithful to the specification? I think a careful review of the functionality is warranted as we have been bitten before by subtle issues in this kind of thing slipping in and causing problems far down the road. But this is also a blocker for parity with GCC on Linux kernel builds in particular and the patch author has gone through a lot of rounds of feedback already, so I'd be quite keen to land this and follow-up with refactoring patches, if my understanding of the situation is correct.

[jrtc27]

I mean, I did comment on the interface 2.5 months ago, that’s not new feedback

[craig.topper]

Looks like reducing the Streamer interface from 3 down to 1 would require refactoring the code to collect the information before sending it to the streamer. I suggest this be moved to a future patch for expediency of getting this landed since the code has already been reviewed. As long as we're the only target using it, it doesn't seem hard to change. I'm willing to help @StephenFan improve this post-commit.

[hiraditya]

In D123515#4372698, @craig.topper wrote:

Looks like reducing the Streamer interface from 3 down to 1 would require refactoring the code to collect the information before sending it to the streamer. I suggest this be moved to a future patch for expediency of getting this landed since the code has already been reviewed. As long as we're the only target using it, it doesn't seem hard to change. I'm willing to help @StephenFan improve this post-commit.

SGTM, Thanks for offering to help Craig!

[StephenFan]

Thank you all! Considering Android developers are really anxious to get this, I will land it tonight if there are no strong objections.

[jrtc27]

llvm/test/MC/RISCV/option-arch.s

94

This wasn't fixed prior to (or while) committing; bacb14b9f31ede0572af504407696c549362c874