[PATCH] D105168: [RISCV] Unify the arch string parsing logic to RISCVISAInfo.

[Kito Cheng via Phabricator via cfe-commits]

kito-cheng marked 8 inline comments as done.
kito-cheng added inline comments.


================
Comment at: clang/test/Driver/riscv-arch.c:198-201
-// RUN: %clang -target riscv32-unknown-elf -march=rv32e -### %s \
-// RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32E %s
-// RV32E: error: invalid arch name 'rv32e',
-// RV32E: standard user-level extension 'e'
----------------
jrtc27 wrote:
> Hm, given we don't currently support RV32E properly this should probably be an error still, but could be a "nicer" error than a generic "invalid arch name" (which is technically wrong)
We support E-extension on MC-layer...but not support `ilp32e`.

This patch unify the ISA stuffs, so either I need to remove that from MC, or I need to made rv32e supported on driver...

https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/RISCV/MCTargetDesc/RISCVTargetStreamer.cpp#L50
https://github.com/llvm/llvm-project/blob/main/llvm/test/MC/RISCV/rv32e-valid.s
https://github.com/llvm/llvm-project/blob/main/llvm/test/MC/RISCV/rv32e-invalid.s


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:40
+
+static const StringRef AllStdExts = "mafdqlcbjtpvn";
+
----------------
jrtc27 wrote:
> craig.topper wrote:
> > Make this `static constexpr StringLiteral AllStdExts = "mafdqlcbjtpvn";`
> I can't help but feel this is really an array of chars, not a string. We don't even need the trailing NUL, though double quote syntax is simpler than curly braces and a bunch of single-quote chars just to save a byte.
Yeah, it's actually just an array of chars, but we have use find function from StringRef :p


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:135
+  if (CheckExperimental)
+    IsExperimental = stripExperimentalPrefix(Ext);
+
----------------
jrtc27 wrote:
> *Extensions* don't have an experimental- prefix, *internal target features* do, so something strange is going on here. This feels like we're confusing several things:
> 1. Whether or not Ext is a feature or an extension
> 2. Whether or not Ext is experimental
> 3. Whether we want to look at experimental extensions
> Some of those are somewhat necessarily interwoven, but the naming does not currently accurately reflect what these things mean, and I would argue we should be very explicit and keep features and extensions separate, never using the same thing to represent both.
Good point, CheckExperimental is kind of ambiguous,  new function: isSupportedExtensionFeature added.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105168