[RISCV] Unify depedency check and extension implication parsing logics

[eopXD]

Summary

Originially there are two places that does parsing - parseArchString and
parseFeatures, each with its code on dependency check and implication.
This patch extracts common parts of the two as functions of RISCVISAInfo
and let them 2 use it.

[eopXD]

Rebase.

[eopXD]

Minor update.

[eopXD]

Remove debug code.

[eopXD]

Update clang-format.

[eopXD]

Since parsing logic is unified, code of RISCVAsmParser can be reduced.

[eopXD]

Fix test fail and clang-format error.

[eopXD]

Update code and test case.

[eopXD]

The test cases are modified because unifying the logic here shows the place where clang / llvm deal with target feature inconsistently.
I think it would be beneficial if we have them consistent.

• Test cases with clang -cc1 originally don't do dependency check to target feature specified
  • This patch add target feature so that dependency checks can pass

Update:2021/10/31
This second part is canceled after Alex's comment below.

• Originally clang driver's -march enforces version to be specified, while llvm allows .attribute arch to not specify version (and picks the default one)
  • This patch lets clang search for default version if no version is specified (so delete NOVERS test case in clang/test/Driver/riscv-arch.c

[asb]

Thank you for the patch. This doesn't apply cleanly to current HEAD, could you please rebase?

I think the second part of this that you list, where the ISA extension version becomes optional is undesirable. We made the explicit choice of requiring the version string to be specified for experimental extensions in order to make it even clearer up-front that these are subject to change (and to allow people to catch easily when the supported ISA version has changed in a compiler update).

[luke957]

So sorry for my bad herald script.

[eopXD]

Rebase

[eopXD]

Address comment: Enforce user to specify version checks.

@asb Thank you for leaving comments for this patch.

[eopXD]

Set ExperimentalExtensionVersionCheck=true for RISCVISAInfo::parseArchString in RISCVAsmParser.cpp

Add version numbers for test case in attribute-arch.s

[eopXD]

rebase.

[eopXD]

Rebase.

[asb]

In D112359#3098960, @eopXD wrote:

Add version numbers for test case in attribute-arch.s

Thanks for updating the patch. Why change this test case? I thought those lines were verifying that .attribute arch without a version number was accepted but the output file included a version number. I'll admit that's not fully consistent with requiring the version number explicitly on the command-line. If a behaviour change is desired here, it would be good to test the case where version numbers aren't specified.

I think for a patch like this, if it at all possible it would be best to do the refactoring in a way that doesn't alter behaviour and then follow-up with separate patches that change any other behaviour (if desired). Then we can review and discuss the refactoring and any functional changes separately, without one discussion blocking the other.

[eopXD]

@asb Thanks for the reply.

To clarify the question, the 2 inconsistencies are:

• Test cases with clang -cc1 originally don't do dependency check to target feature specified (handled by parseFeatures)
• Clang driver's -march (handled by parseArchString) enforces version to be specified, while llvm allows .attribute arch to not specify version and picks the default one (handled by parseFeatures)

What this patch tries to do, letting the 2 actions rely on the same function call will mean to alter the behavior on one of them.
In my humble opinion, it is rather awkward to make this patch an NFC while letting the 2 actions call the same function.
Maybe we can first come to a conclusion on how do we unify and then implement them in this patch.

The main intentions of unifying them is that there will be some implications and dependency checks on the addition of zve and zvl,
so unifying the logics will make the addition have less code duplication.

[eopXD]

ping, thank you.

[asb]

OK, that reasoning makes sense. I think my only outstanding request would be to ensure there's some test coverage for the case of .attribute arch with an experimental extension without version info.

[eopXD]

• Update code (zfh have been added recently.
• Added test case attribute-arch-invalid.s to show that experimental extensions need to specify the version explicitly.

[eopXD]

One last rebase before landing.

clang/test/CodeGen/RISCV/riscv-metadata.c

 // RUN: %clang_cc1 -triple riscv32 -target-abi ilp32 -emit-llvm -o - %s | FileCheck -check-prefix=ILP32 %s
 // RUN: %clang_cc1 -triple riscv32 -target-feature +f -target-abi ilp32f -emit-llvm -o - %s | FileCheck -check-prefix=ILP32F %s
-// RUN: %clang_cc1 -triple riscv32 -target-feature +d -target-abi ilp32d -emit-llvm -o - %s | FileCheck -check-prefix=ILP32D %s
+// RUN: %clang_cc1 -triple riscv32 -target-feature +d -target-feature +f -target-abi ilp32d -emit-llvm -o - %s | FileCheck -check-prefix=ILP32D %s
 // RUN: %clang_cc1 -triple riscv64 -target-abi lp64 -emit-llvm -o - %s | FileCheck -check-prefix=LP64 %s
 // RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-abi lp64f -emit-llvm -o - %s | FileCheck -check-prefix=LP64F %s
-// RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-abi lp64d -emit-llvm -o - %s | FileCheck -check-prefix=LP64D %s
+// RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-feature +f -target-abi lp64d -emit-llvm -o - %s | FileCheck -check-prefix=LP64D %s
 
 // ILP32: !{{[0-9]+}} = !{i32 1, !"target-abi", !"ilp32"}
 // ILP32F: !{{[0-9]+}} = !{i32 1, !"target-abi", !"ilp32f"}
 // ILP32D: !{{[0-9]+}} = !{i32 1, !"target-abi", !"ilp32d"}
 
 // LP64: !{{[0-9]+}} = !{i32 1, !"target-abi", !"lp64"}
 // LP64F: !{{[0-9]+}} = !{i32 1, !"target-abi", !"lp64f"}
 // LP64D: !{{[0-9]+}} = !{i32 1, !"target-abi", !"lp64d"}

clang/test/CodeGen/RISCV/riscv32-ilp32-ilp32f-ilp32d-abi.c

 // RUN: %clang_cc1 -triple riscv32 -emit-llvm %s -o - | FileCheck %s
 // RUN: %clang_cc1 -triple riscv32 -emit-llvm -fforce-enable-int128 %s -o - \
 // RUN:   | FileCheck %s -check-prefixes=CHECK,CHECK-FORCEINT128
 // RUN: %clang_cc1 -triple riscv32 -target-feature +f -target-abi ilp32f -emit-llvm %s -o - \
 // RUN:     | FileCheck %s
-// RUN: %clang_cc1 -triple riscv32 -target-feature +d -target-abi ilp32d -emit-llvm %s -o - \
+// RUN: %clang_cc1 -triple riscv32 -target-feature +d -target-feature +f -target-abi ilp32d -emit-llvm %s -o - \
 // RUN:     | FileCheck %s
 
 // This file contains test cases that will have the same output for the ilp32,
 // ilp32f, and ilp32d ABIs.
 
 #include <stddef.h>
 #include <stdint.h>
 

clang/test/CodeGen/RISCV/riscv32-ilp32d-abi.c

-// RUN: %clang_cc1 -triple riscv32 -target-feature +d -target-abi ilp32d -emit-llvm %s -o - \
+// RUN: %clang_cc1 -triple riscv32 -target-feature +d -target-feature +f -target-abi ilp32d -emit-llvm %s -o - \
 // RUN:     | FileCheck %s
 
 #include <stdint.h>
 
 // Verify that the tracking of used GPRs and FPRs works correctly by checking
 // that small integers are sign/zero extended when passed in registers.
 
 // Doubles are passed in FPRs, so argument 'i' will be passed zero-extended

clang/test/CodeGen/RISCV/riscv32-ilp32f-ilp32d-abi.c

 // RUN: %clang_cc1 -triple riscv32 -target-feature +f -target-abi ilp32f -emit-llvm %s -o - \
 // RUN:     | FileCheck %s
-// RUN: %clang_cc1 -triple riscv32 -target-feature +d -target-abi ilp32d -emit-llvm %s -o - \
+// RUN: %clang_cc1 -triple riscv32 -target-feature +d -target-feature +f -target-abi ilp32d -emit-llvm %s -o - \
 // RUN:     | FileCheck %s
 
 #include <stdint.h>
 
 // Verify that the tracking of used GPRs and FPRs works correctly by checking
 // that small integers are sign/zero extended when passed in registers.
 
 // Floats are passed in FPRs, so argument 'i' will be passed zero-extended

clang/test/CodeGen/RISCV/riscv64-lp64-lp64f-lp64d-abi.c

 // RUN: %clang_cc1 -triple riscv64 -emit-llvm %s -o - | FileCheck %s
 // RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-abi lp64f -emit-llvm %s -o - \
 // RUN:     | FileCheck %s
-// RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-abi lp64d -emit-llvm %s -o - \
+// RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-feature +f -target-abi lp64d -emit-llvm %s -o - \
 // RUN:     | FileCheck %s
 
 // This file contains test cases that will have the same output for the lp64,
 // lp64f, and lp64d ABIs.
 
 #include <stddef.h>
 #include <stdint.h>
 

clang/test/CodeGen/RISCV/riscv64-lp64d-abi.c

-// RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-abi lp64d -emit-llvm %s -o - \
+// RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-feature +f -target-abi lp64d -emit-llvm %s -o - \
 // RUN:     | FileCheck %s
 
 #include <stdint.h>
 
 // Verify that the tracking of used GPRs and FPRs works correctly by checking
 // that small integers are sign/zero extended when passed in registers.
 
 // Doubles are passed in FPRs, so argument 'i' will be passed zero-extended

clang/test/CodeGen/RISCV/riscv64-lp64f-lp64d-abi.c

 // RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-abi lp64f -emit-llvm %s -o - \
 // RUN:     | FileCheck %s
-// RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-abi lp64d -emit-llvm %s -o - \
+// RUN: %clang_cc1 -triple riscv64 -target-feature +d -target-feature +f -target-abi lp64d -emit-llvm %s -o - \
 // RUN:     | FileCheck %s
 
 #include <stdint.h>
 
 // Verify that the tracking of used GPRs and FPRs works correctly by checking
 // that small integers are sign/zero extended when passed in registers.
 
 // Floats are passed in FPRs, so argument 'i' will be passed zero-extended

clang/test/CodeGen/riscv32-ilp32d-abi.cpp

-// RUN: %clang_cc1 -triple riscv32 -target-feature +d -target-abi ilp32d \
+// RUN: %clang_cc1 -triple riscv32 -target-feature +d -target-feature +f -target-abi ilp32d \
 // RUN:     -Wno-missing-declarations -emit-llvm %s -o - | FileCheck %s
 
 struct empty_float2 { struct {}; float f; float g; };
 
 // CHECK: define{{.*}} float @_Z14f_empty_float212empty_float2(float %0, float %1)
 // CHECK: { [4 x i8], float, float }
 float f_empty_float2(empty_float2 a) {
     return a.g;

llvm/include/llvm/Support/RISCVISAInfo.h

private:
   unsigned XLen;
   unsigned FLen;
 
   OrderedExtensionMap Exts;
 
   void addExtension(StringRef ExtName, unsigned MajorVersion,
                     unsigned MinorVersion);
 
+  Error checkDependency();
+
+  void updateImplication();
   void updateFLen();
 };
 
 } // namespace llvm
 
 #endif

llvm/lib/Support/RISCVISAInfo.cpp

 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Errc.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include <array>
 #include <string>
 #include <vector>
+#include <iostream>
 
 using namespace llvm;
 
 namespace {
 /// Represents the major and version number components of a RISC-V extension
 struct RISCVExtensionVersion {
   unsigned Major;
   unsigned Minor;

 }
 
 llvm::Expected<std::unique_ptr<RISCVISAInfo>>
 RISCVISAInfo::parseFeatures(unsigned XLen,
                             const std::vector<std::string> &Features) {
   assert(XLen == 32 || XLen == 64);
   std::unique_ptr<RISCVISAInfo> ISAInfo(new RISCVISAInfo(XLen));
 
-  bool HasE = false;
   for (auto &Feature : Features) {
     StringRef ExtName = Feature;
     bool Experimental = false;
     assert(ExtName.size() > 1 && (ExtName[0] == '+' || ExtName[0] == '-'));
     bool Add = ExtName[0] == '+';
     ExtName = ExtName.drop_front(1); // Drop '+' or '-'
     Experimental = stripExperimentalPrefix(ExtName);
     auto ExtensionInfos = Experimental
                               ? makeArrayRef(SupportedExperimentalExtensions)
                               : makeArrayRef(SupportedExtensions);
     auto ExtensionInfoIterator =
         llvm::find_if(ExtensionInfos, FindByName(ExtName));
 
     // Not all features is related to ISA extension, like `relax` or
     // `save-restore`, skip those feature.
     if (ExtensionInfoIterator == ExtensionInfos.end())
       continue;
 
-    if (Add) {
-      if (ExtName == "e") {
-        if (XLen != 32)
-          return createStringError(
-              errc::invalid_argument,
-              "standard user-level extension 'e' requires 'rv32'");
-        HasE = true;
-      }
-
+    if (Add)
       ISAInfo->addExtension(ExtName, ExtensionInfoIterator->Version.Major,
                             ExtensionInfoIterator->Version.Minor);
-    } else
-      ISAInfo->Exts.erase(ExtName.str());
-  }
-  if (!HasE) {
-    if (auto Version = findDefaultVersion("i"))
-      ISAInfo->addExtension("i", Version->Major, Version->Minor);
     else
-      llvm_unreachable("Default extension version for 'i' not found?");
+      ISAInfo->Exts.erase(ExtName.str());
   }
 
   ISAInfo->updateFLen();
+  ISAInfo->updateImplication();
+
+  if (Error Result = ISAInfo->checkDependency())
+    return std::move(Result);
 
   return std::move(ISAInfo);
 }
 
 llvm::Expected<std::unique_ptr<RISCVISAInfo>>
 RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
                               bool ExperimentalExtensionVersionCheck) {
   // RISC-V ISA strings must be lowercase.
RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
   }
 
   unsigned XLen = HasRV64 ? 64 : 32;
   std::unique_ptr<RISCVISAInfo> ISAInfo(new RISCVISAInfo(XLen));
 
   // The canonical order specified in ISA manual.
   // Ref: Table 22.1 in RISC-V User-Level ISA V2.2
   StringRef StdExts = AllStdExts;
-  bool HasF = false, HasD = false;
   char Baseline = Arch[4];
 
   // First letter should be 'e', 'i' or 'g'.
   switch (Baseline) {
   default:
     return createStringError(errc::invalid_argument,
                              "first letter should be 'e', 'i' or 'g'");
   case 'e': {
     // Extension 'e' is not allowed in rv64.
     if (HasRV64)
       return createStringError(
           errc::invalid_argument,
           "standard user-level extension 'e' requires 'rv32'");
     break;
   }
   case 'i':
     break;
   case 'g':
     // g = imafd
     StdExts = StdExts.drop_front(4);
-    HasF = true;
-    HasD = true;
     break;
   }
 
   // Skip rvxxx
   StringRef Exts = Arch.substr(5);
 
   // Remove multi-letter standard extensions, non-standard extensions and
   // supervisor-level extensions. They have 'z', 'x', 's', 'sx' prefixes.
   // Parse them at the end.
   // Find the very first occurrence of 's', 'x' or 'z'.
   StringRef OtherExts;
   size_t Pos = Exts.find_first_of("zsx");
   if (Pos != StringRef::npos) {
     OtherExts = Exts.substr(Pos);
     Exts = Exts.substr(0, Pos);
   }
 
+  dbgs() << OtherExts << "\n";
+
   unsigned Major, Minor, ConsumeLength;
   if (auto E = getExtensionVersion(std::string(1, Baseline), Exts, Major, Minor,
                                    ConsumeLength, EnableExperimentalExtension,
                                    ExperimentalExtensionVersionCheck))
     return std::move(E);
 
   if (Baseline == 'g') {
     // No matter which version is given to `g`, we always set imafd to default
if (std::next(I) != E)
       Next = std::string(std::next(I), E);
     if (auto E = getExtensionVersion(std::string(1, C), Next, Major, Minor,
                                      ConsumeLength, EnableExperimentalExtension,
                                      ExperimentalExtensionVersionCheck))
       return std::move(E);
 
     // The order is OK, then push it into features.
     // TODO: Use version number when setting target features
-    switch (C) {
-    default:
-      // Currently LLVM supports only "mafdcbv".
+    StringRef SupportedStandardExtension = "mafdcbv";
+    if (SupportedStandardExtension.find(C) == StringRef::npos)
       return createStringError(errc::invalid_argument,
                                "unsupported standard user-level extension '%c'",
                                C);
-    case 'm':
-      ISAInfo->addExtension("m", Major, Minor);
-      break;
-    case 'a':
-      ISAInfo->addExtension("a", Major, Minor);
-      break;
-    case 'f':
-      ISAInfo->addExtension("f", Major, Minor);
-      HasF = true;
-      break;
-    case 'd':
-      ISAInfo->addExtension("d", Major, Minor);
-      HasD = true;
-      break;
-    case 'c':
-      ISAInfo->addExtension("c", Major, Minor);
-      break;
-    case 'v':
-      ISAInfo->addExtension("v", Major, Minor);
-      ISAInfo->addExtension("zvlsseg", Major, Minor);
-      break;
-    }
+    ISAInfo->addExtension(std::string(1, C), Major, Minor);
+
     // Consume full extension name and version, including any optional '_'
     // between this extension and the next
     ++I;
     I += ConsumeLength;
     if (*I == '_')
       ++I;
   }
-  // Dependency check.
-  // It's illegal to specify the 'd' (double-precision floating point)
-  // extension without also specifying the 'f' (single precision
-  // floating-point) extension.
-  // TODO: This has been removed in later specs, which specify that D implies F
-  if (HasD && !HasF)
-    return createStringError(errc::invalid_argument,
-                             "d requires f extension to also be specified");
-
-  // Additional dependency checks.
-  // TODO: The 'q' extension requires rv64.
-  // TODO: It is illegal to specify 'e' extensions with 'f' and 'd'.
-
-  if (OtherExts.empty())
-    return std::move(ISAInfo);
 
   // Handle other types of extensions other than the standard
   // general purpose and standard user-level extensions.
   // 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', 'x', 's', 'sx' prefixes, follow a
   // canonical order, might have a version number (major, minor)
   // and are separated by a single underscore '_'.
   // Set the hardware features for the extensions that are supported.
 
   // Multi-letter extensions are seperated by a single underscore
   // as described in RISC-V User-Level ISA V2.2.
   SmallVector<StringRef, 8> Split;
   OtherExts.split(Split, '_');
 
   SmallVector<StringRef, 8> AllExts;
   std::array<StringRef, 4> Prefix{"z", "x", "s", "sx"};
   auto I = Prefix.begin();
   auto E = Prefix.end();
-
+  if (Split.size() > 1 || Split[0] != "") {
     for (StringRef Ext : Split) {
       if (Ext.empty())
         return createStringError(errc::invalid_argument,
                                 "extension name missing after separator '_'");
 
       StringRef Type = getExtensionType(Ext);
       StringRef Desc = getExtensionTypeDesc(Ext);
-    size_t Pos = findFirstNonVersionCharacter(Ext) + 1;
+      auto Pos = findFirstNonVersionCharacter(Ext) + 1;
       StringRef Name(Ext.substr(0, Pos));
       StringRef Vers(Ext.substr(Pos));
 
       if (Type.empty())
         return createStringError(errc::invalid_argument,
                                 "invalid extension prefix '" + Ext + "'");
 
       // Check ISA extensions are specified in the canonical order.
       while (I != E && *I != Type)
         ++I;
 
       if (I == E)
         return createStringError(errc::invalid_argument,
                                 "%s not given in canonical order '%s'",
                                 Desc.str().c_str(), Ext.str().c_str());
 
       if (Name.size() == Type.size()) {
         return createStringError(errc::invalid_argument,
                                 "%s name missing after '%s'", Desc.str().c_str(),
                                 Type.str().c_str());
       }
 
       unsigned Major, Minor, ConsumeLength;
       if (auto E = getExtensionVersion(Name, Vers, Major, Minor, ConsumeLength,
                                       EnableExperimentalExtension,
                                       ExperimentalExtensionVersionCheck))
         return std::move(E);
 
       // Check if duplicated extension.
       if (llvm::is_contained(AllExts, Name))
         return createStringError(errc::invalid_argument, "duplicated %s '%s'",
                                 Desc.str().c_str(), Name.str().c_str());
 
       ISAInfo->addExtension(Name, Major, Minor);
       // Extension format is correct, keep parsing the extensions.
       // TODO: Save Type, Name, Major, Minor to avoid parsing them later.
       AllExts.push_back(Name);
     }
+  }
 
   for (auto Ext : AllExts) {
     if (!isSupportedExtension(Ext)) {
       StringRef Desc = getExtensionTypeDesc(getExtensionType(Ext));
       return createStringError(errc::invalid_argument, "unsupported %s '%s'",
                                Desc.str().c_str(), Ext.str().c_str());
     }
   }
 
+  ISAInfo->updateImplication();
   ISAInfo->updateFLen();
 
+  if (Error Result = ISAInfo->checkDependency())
+    return std::move(Result);
+
   return std::move(ISAInfo);
 }
 
+Error RISCVISAInfo::checkDependency() {
+  bool IsRv32 = XLen == 32;
+  bool HasE = Exts.count("e") == 1;
+  bool HasD = Exts.count("d") == 1;
+  bool HasF = Exts.count("f") == 1;
+
+  if (HasE && !IsRv32)
+    return createStringError(
+      errc::invalid_argument,
+      "standard user-level extension 'e' requires 'rv32'");
+
+  // It's illegal to specify the 'd' (double-precision floating point)
+  // extension without also specifying the 'f' (single precision
+  // floating-point) extension.
+  // TODO: This has been removed in later specs, which specify that D implies F
+  if (HasD && !HasF)
+    return createStringError(
+        errc::invalid_argument,
+        "d requires f extension to also be specified");
+
+  // Additional dependency checks.
+  // TODO: The 'q' extension requires rv64.
+  // TODO: It is illegal to specify 'e' extensions with 'f' and 'd'.
+
+  return Error::success();
+}
+
+void RISCVISAInfo::updateImplication() {
+  bool HasE = Exts.count("e") == 1;
+  bool HasI = Exts.count("i") == 1;
+  bool HasV = Exts.count("v") == 1;
+
+  // If not in e extension and i extension does not exist, i extension is
+  // implied
+  if (!HasE && !HasI) {
+    auto Version = findDefaultVersion("i");
+    addExtension("i", Version->Major, Version->Minor);
+  }
+
+  if (HasV) {
+    auto Version = findDefaultVersion("zvlsseg");
+    addExtension("zvlsseg", Version->Major, Version->Minor);
+  }
+}
+
 void RISCVISAInfo::updateFLen() {
   FLen = 0;
   // TODO: Handle q extension.
   if (Exts.count("d"))
     FLen = 64;
   else if (Exts.count("f"))
     FLen = 32;
 }