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

[Luís Marques via Phabricator via llvm-commits]

luismarques added a comment.

You now accept `.option arch, rv32ic` (without the quotes) but that's not covered in the valid tests.
Also, you're normalizing that to the quoted version `.option arch, "rv32i2p0_c2p0"` and accepting that but, at first glance, I'm not seeing that as an accepted case in the binutils tests.

The test coverage could be extended a bit to cover e.g.:

- Test accepted cases with comma-separated lists (e.g. `+c, +m`)
- Test invalid attempt to remove `i` base ISA
- Test `+c, -c` (and vice-versa), see if it does the correct thing
- Test multi-letter extensions like `+zba`
- Test full ISA string, with and without versions



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:185-187
+  /// Helper to reset target features for a new arch string. It
+  /// also records the new arch string that expanded by RISCVISAInfo
+  /// and reports error for invalid arch string
----------------
Two nits:
"that expanded" -> "that is expanded"?
Missing period.


================
Comment at: llvm/test/MC/RISCV/option-arch.s:94
+
+# Test '.option arch, <arch-stringg>' directive
+#CHECK: .option arch, "rv32i2p0_m2p0_a2p0_c2p0"
----------------
Nit: "stringg"


================
Comment at: llvm/test/MC/RISCV/option-arch.s:96
+#CHECK: .option arch, "rv32i2p0_m2p0_a2p0_c2p0"
+.option arch, "rv32i2p0_m2p0_a2p0_c2p0"
----------------
You're still accepting quoted ISA strings and not testing unquoted ISA strings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123515