[RISCV] Unify depedency check and extension implication parsing logics

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. Reviewed By: asb Differential Revision: https://reviews.llvm.org/D112359
llvm · Dec 10, 2021 · a4bf1b4 · a4bf1b4
1 parent e308b8e
commit a4bf1b4
Show file tree

Hide file tree

Showing 13 changed files with 194 additions and 160 deletions.
diff --git a/clang/test/CodeGen/RISCV/riscv-metadata.c b/clang/test/CodeGen/RISCV/riscv-metadata.c
@@ -1,9 +1,9 @@
 // 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"}

diff --git a/clang/test/CodeGen/RISCV/riscv32-ilp32-ilp32f-ilp32d-abi.c b/clang/test/CodeGen/RISCV/riscv32-ilp32-ilp32f-ilp32d-abi.c
@@ -3,7 +3,7 @@
 // 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,

diff --git a/clang/test/CodeGen/RISCV/riscv32-ilp32d-abi.c b/clang/test/CodeGen/RISCV/riscv32-ilp32d-abi.c
@@ -1,4 +1,4 @@
-// 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>

diff --git a/clang/test/CodeGen/RISCV/riscv32-ilp32f-ilp32d-abi.c b/clang/test/CodeGen/RISCV/riscv32-ilp32f-ilp32d-abi.c
@@ -1,6 +1,6 @@
 // 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>

diff --git a/clang/test/CodeGen/RISCV/riscv64-lp64-lp64f-lp64d-abi.c b/clang/test/CodeGen/RISCV/riscv64-lp64-lp64f-lp64d-abi.c
@@ -1,7 +1,7 @@
 // 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,

diff --git a/clang/test/CodeGen/RISCV/riscv64-lp64d-abi.c b/clang/test/CodeGen/RISCV/riscv64-lp64d-abi.c
@@ -1,4 +1,4 @@
-// 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>

diff --git a/clang/test/CodeGen/RISCV/riscv64-lp64f-lp64d-abi.c b/clang/test/CodeGen/RISCV/riscv64-lp64f-lp64d-abi.c
@@ -1,6 +1,6 @@
 // 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>

diff --git a/clang/test/CodeGen/riscv32-ilp32d-abi.cpp b/clang/test/CodeGen/riscv32-ilp32d-abi.cpp
@@ -1,4 +1,4 @@
-// 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; };

diff --git a/llvm/include/llvm/Support/RISCVISAInfo.h b/llvm/include/llvm/Support/RISCVISAInfo.h
@@ -81,6 +81,9 @@ class RISCVISAInfo {
   void addExtension(StringRef ExtName, unsigned MajorVersion,
                     unsigned MinorVersion);
 
+  Error checkDependency();
+
+  void updateImplication();
   void updateFLen();
 };
 

diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -412,7 +412,6 @@ RISCVISAInfo::parseFeatures(unsigned XLen,
   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;
@@ -431,29 +430,19 @@ RISCVISAInfo::parseFeatures(unsigned XLen,
     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->updateImplication();
   ISAInfo->updateFLen();
 
+  if (Error Result = ISAInfo->checkDependency())
+    return std::move(Result);
+
   return std::move(ISAInfo);
 }
 
@@ -479,7 +468,6 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
   // 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'.
@@ -500,8 +488,6 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
   case 'g':
     // g = imafd
     StdExts = StdExts.drop_front(4);
-    HasF = true;
-    HasD = true;
     break;
   }
 
@@ -582,56 +568,21 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
 
     // 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".
+    // 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.
@@ -652,52 +603,53 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
   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);
+      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());
+      }
 
-  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;
-    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);
     }
-
-    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) {
@@ -708,11 +660,65 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
     }
   }
 
+  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;
+  bool HasZfh = Exts.count("zfh") == 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);
+  }
+
+  if (HasZfh) {
+    auto Version = findDefaultVersion("zfhmin");
+    addExtension("zfhmin", Version->Major, Version->Minor);
+  }
+}
+
 void RISCVISAInfo::updateFLen() {
   FLen = 0;
   // TODO: Handle q extension.