From 6101d720cb499f5ad19293475b429828fa3dbd75 Mon Sep 17 00:00:00 2001 From: Alex Bradbury Date: Tue, 27 Jun 2023 13:28:28 +0100 Subject: [PATCH] [RISCV] Relax rules for ordering s/z/x prefixed extensions in ISA naming strings This was discussed somewhat in D148315. As it stands, we require in RISCVISAInfo::parseArchString (used for e.g. -march parsing in Clang) that extensions are given in the order of z, then s, then x prefixed extensions (after the standard single-letter extensions). However, we recently (in D148315) moved to that order from z/x/s as the canonical ordering was changed in the spec. In addition, recent GCC seems to require z* extensions before s*. My recollection of the history here is that we thought keeping -march as close to the rules for ISA naming strings as possible would simplify things, as there's an existing spec to point to. My feeling is that now we've had incompatible changes, and an incompatibility with GCC there's no real benefit to sticking to this restriction, and it risks making it much more painful than it needs to be to copy a -march= string between GCC and Clang. This patch removes all ordering restrictions so you can freely mix x/s/z extensions. To be very explicit, this doesn't change our behaviour when emitting a canonically ordered extension string (e.g. in build attributes). We of course sort according to the canonical order (as we understand it) in that case. Differential Revision: https://reviews.llvm.org/D149246 --- clang/docs/ReleaseNotes.rst | 3 +++ clang/test/Driver/riscv-arch.c | 3 +-- llvm/docs/RISCVUsage.rst | 2 ++ llvm/lib/Support/RISCVISAInfo.cpp | 21 +++------------------ llvm/unittests/Support/RISCVISAInfoTest.cpp | 10 ++++------ 5 files changed, 13 insertions(+), 26 deletions(-) diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst index bc6366b..1ecf620 100644 --- a/clang/docs/ReleaseNotes.rst +++ b/clang/docs/ReleaseNotes.rst @@ -704,6 +704,9 @@ RISC-V Support - Added ``attribute(riscv_rvv_vector_bits(__riscv_v_fixed_vlen))`` to allow the size of a RVV (RISC-V Vector) scalable type to be specified. This allows RVV scalable vector types to be used in structs or in global variables. +- The rules for ordering of extensions in ``-march`` strings were relaxed. A + canonical ordering is no longer enforced on ``z*``, ``s*``, and ``x*`` + prefixed extensions. CUDA/HIP Language Changes ^^^^^^^^^^^^^^^^^^^^^^^^^ diff --git a/clang/test/Driver/riscv-arch.c b/clang/test/Driver/riscv-arch.c index 8929d88..4f618d5 100644 --- a/clang/test/Driver/riscv-arch.c +++ b/clang/test/Driver/riscv-arch.c @@ -323,8 +323,7 @@ // RUN: %clang --target=riscv32-unknown-elf -march=rv32ixdef_sabc -### %s \ // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-X-ORDER %s // RV32-X-ORDER: error: invalid arch name 'rv32ixdef_sabc', -// RV32-X-ORDER: standard supervisor-level extension not given -// RV32-X-ORDER: in canonical order 'sabc' +// RV32-X-ORDER unsupported non-standard user-level extension 'xdef' // RUN: %clang --target=riscv32-unknown-elf -march=rv32ixabc_xabc -### %s \ // RUN: -fsyntax-only 2>&1 | FileCheck -check-prefix=RV32-XDUP %s diff --git a/llvm/docs/RISCVUsage.rst b/llvm/docs/RISCVUsage.rst index 66c4aa6..f50ddf2 100644 --- a/llvm/docs/RISCVUsage.rst +++ b/llvm/docs/RISCVUsage.rst @@ -40,6 +40,8 @@ The current known variances from the specification are: users migrate build systems so as not to rely on this. * Allowing CSRs to be named without gating on specific extensions. This applies to all CSR names, not just those in zicsr, zicntr, and zihpm. +* The ordering of ``z*``, ``s*``, and ``x*`` prefixed extension names is not + enforced in user-specified ISA naming strings (e.g. ``-march``). We are actively deciding not to support multiple specification revisions at this time. We acknowledge a likely future need, but actively defer the diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp index 58a98bc..7b12abc 100644 --- a/llvm/lib/Support/RISCVISAInfo.cpp +++ b/llvm/lib/Support/RISCVISAInfo.cpp @@ -811,9 +811,9 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension, // Parse the ISA string containing non-standard user-level // extensions, standard supervisor-level extensions and // non-standard supervisor-level extensions. - // These extensions start with 'z', 's', 'x' prefixes, follow a - // canonical order, might have a version number (major, minor) - // and are separated by a single underscore '_'. + // These extensions start with 'z', 's', 'x' prefixes, might have a version + // number (major, minor) and are separated by a single underscore '_'. We do + // not enforce a canonical order for them. // Set the hardware features for the extensions that are supported. // Multi-letter extensions are seperated by a single underscore @@ -822,9 +822,6 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension, OtherExts.split(Split, '_'); SmallVector AllExts; - std::array Prefix{"z", "s", "x"}; - auto I = Prefix.begin(); - auto E = Prefix.end(); if (Split.size() > 1 || Split[0] != "") { for (StringRef Ext : Split) { if (Ext.empty()) @@ -844,18 +841,6 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension, "invalid extension prefix '" + Ext + "'"); } - // Check ISA extensions are specified in the canonical order. - while (I != E && *I != Type) - ++I; - - if (I == E) { - if (IgnoreUnknown) - continue; - return createStringError(errc::invalid_argument, - "%s not given in canonical order '%s'", - Desc.str().c_str(), Ext.str().c_str()); - } - if (!IgnoreUnknown && Name.size() == Type.size()) { return createStringError(errc::invalid_argument, "%s name missing after '%s'", diff --git a/llvm/unittests/Support/RISCVISAInfoTest.cpp b/llvm/unittests/Support/RISCVISAInfoTest.cpp index 71e0d75..cab91c4 100644 --- a/llvm/unittests/Support/RISCVISAInfoTest.cpp +++ b/llvm/unittests/Support/RISCVISAInfoTest.cpp @@ -192,7 +192,7 @@ TEST(ParseArchString, AcceptsSupportedBaseISAsAndSetsXLenAndFLen) { EXPECT_EQ(InfoRV64G.getFLen(), 64U); } -TEST(ParseArchString, RequiresCanonicalOrderForExtensions) { +TEST(ParseArchString, RequiresCanonicalOrderForSingleLetterExtensions) { EXPECT_EQ( toString(RISCVISAInfo::parseArchString("rv64idf", true).takeError()), "standard user-level extension not given in canonical order 'f'"); @@ -203,12 +203,10 @@ TEST(ParseArchString, RequiresCanonicalOrderForExtensions) { toString( RISCVISAInfo::parseArchString("rv32i_zfinx_a", true).takeError()), "invalid extension prefix 'a'"); - EXPECT_EQ( - toString(RISCVISAInfo::parseArchString("rv64i_svnapot_zicsr", true) - .takeError()), - "standard user-level extension not given in canonical order 'zicsr'"); + // Canonical ordering not required for z*, s*, and x* extensions. EXPECT_THAT_EXPECTED( - RISCVISAInfo::parseArchString("rv64imafdc_zicsr_svnapot", true), + RISCVISAInfo::parseArchString( + "rv64imafdc_xsfvcp_zicsr_xtheadba_svnapot_zawrs", true), Succeeded()); } -- 2.7.4