Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[RISCV] Support '.option arch' directive
ClosedPublic

Authored by StephenFan on Apr 11 2022, 7:53 AM.

Details

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Address comments.

What happened to your new test?

jrtc27 added inline comments.Mar 11 2023, 12:55 AM
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

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.

Address comments and add back test file.

Grammar fixing.

luismarques added inline comments.Mar 24 2023, 6:54 AM
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 accepted this revision.Mar 24 2023, 7:24 AM

Overall LGTM. Only some minor points left to address.

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

Nit: unneeded empty line space.

This revision is now accepted and ready to land.Mar 24 2023, 7:24 AM
luismarques added inline comments.Mar 24 2023, 8:29 AM
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 added a comment.Mar 24 2023, 8:31 AM

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).

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 added inline comments.Mar 24 2023, 12:13 PM
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:

Address comments.

luismarques requested changes to this revision.Apr 6 2023, 3:05 AM

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.
  1. 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.

This revision now requires changes to proceed.Apr 6 2023, 3:05 AM
StephenFan updated this revision to Diff 519010.May 3 2023, 1:36 AM

Reject +relax

StephenFan updated this revision to Diff 519012.May 3 2023, 1:53 AM

Reject switching between rv32 and rv64

StephenFan updated this revision to Diff 519013.May 3 2023, 1:54 AM

Reject switching between rv32 and rv64

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 added a comment.May 3 2023, 1:57 AM

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.

pirama added a subscriber: pirama.May 10 2023, 9:17 AM
craig.topper added inline comments.May 10 2023, 9:50 AM
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 -?

evandro removed a subscriber: evandro.May 17 2023, 3:47 PM

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 accepted this revision.May 17 2023, 10:50 PM

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 requested changes to this revision.May 17 2023, 11:16 PM

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

This revision now requires changes to proceed.May 17 2023, 11:16 PM

Accept extension name that has digits.

StephenFan added inline comments.May 18 2023, 7:40 PM
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 added inline comments.May 19 2023, 1:06 AM
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.

Add test that check impliest transtively

Check dependency when enabling an extension.

StephenFan added inline comments.May 22 2023, 11:14 PM
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 added inline comments.May 22 2023, 11:30 PM
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 added inline comments.May 22 2023, 11:32 PM
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 added inline comments.May 23 2023, 7:50 AM
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 added inline comments.May 23 2023, 9:33 AM
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.

@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 added a comment.EditedMay 24 2023, 6:47 PM

@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 added a comment.May 25 2023, 8:21 AM

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.

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

luismarques accepted this revision.May 25 2023, 8:45 AM

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.

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!

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

This revision was not accepted when it landed; it landed in state Needs Review.May 26 2023, 3:40 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
jrtc27 added inline comments.Jun 5 2023, 1:25 PM
llvm/test/MC/RISCV/option-arch.s
94

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