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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
