[PATCH] D123515: [RISCV] Support '.option arch' directive

Fri Mar 24 08:31:55 PDT 2023

asb added a comment.

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.

================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp:143
+
+void RISCVTargetAsmStreamer::emitDirectiveOptionArchEqual(StringRef Value,
+                                                          bool &PrefixEmitted,
----------------
Maybe emitDirectiveOptionArchFullArch or similar? As `=` isn't part of the grammar in the committed version of the spec.

================
Comment at: llvm/test/MC/RISCV/option-invalid.s:17
+# CHECK: :[[#@LINE+1]]:23: error: unexpected token, expected + or -
+.option arch, +f, +d, rv32ifd, -d
+
----------------
It would be good to check `.option arch rv32ifd, +f, +d` or similar (i.e. an input with a fullarch string coming first).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123515/new/

https://reviews.llvm.org/D123515