[RISCV] Remove IgnoreUnknown from RISCVISAInfo::parseArchString.

[topperc]

This isn't used in tree, and thus I don't know what the expectations for its behavior really are.

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

This isn't used in tree, and thus I don't know what the expectations for its behavior really are.

---

Full diff: https://github.com/llvm/llvm-project/pull/97372.diff

3 Files Affected:

• (modified) llvm/include/llvm/TargetParser/RISCVISAInfo.h (+1-2)
• (modified) llvm/lib/TargetParser/RISCVISAInfo.cpp (+8-28)
• (modified) llvm/unittests/TargetParser/RISCVISAInfoTest.cpp (-48)

diff --git a/llvm/include/llvm/TargetParser/RISCVISAInfo.h b/llvm/include/llvm/TargetParser/RISCVISAInfo.h
index 5e9cf67eddcfd..ba2965600decd 100644
--- a/llvm/include/llvm/TargetParser/RISCVISAInfo.h
+++ b/llvm/include/llvm/TargetParser/RISCVISAInfo.h
@@ -36,8 +36,7 @@ class RISCVISAInfo {
   /// default version will be used (as ignoring the base is not possible).
   static llvm::Expected<std::unique_ptr<RISCVISAInfo>>
   parseArchString(StringRef Arch, bool EnableExperimentalExtension,
-                  bool ExperimentalExtensionVersionCheck = true,
-                  bool IgnoreUnknown = false);
+                  bool ExperimentalExtensionVersionCheck = true);
 
   /// Parse RISC-V ISA info from an arch string that is already in normalized
   /// form (as defined in the psABI). Unlike parseArchString, this function
diff --git a/llvm/lib/TargetParser/RISCVISAInfo.cpp b/llvm/lib/TargetParser/RISCVISAInfo.cpp
index 869be57928890..b4fd067a1ed7a 100644
--- a/llvm/lib/TargetParser/RISCVISAInfo.cpp
+++ b/llvm/lib/TargetParser/RISCVISAInfo.cpp
@@ -522,8 +522,7 @@ RISCVISAInfo::parseNormalizedArchString(StringRef Arch) {
 
 llvm::Expected<std::unique_ptr<RISCVISAInfo>>
 RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
-                              bool ExperimentalExtensionVersionCheck,
-                              bool IgnoreUnknown) {
+                              bool ExperimentalExtensionVersionCheck) {
   // RISC-V ISA strings must be [a-z0-9_]
   if (!llvm::all_of(
           Arch, [](char C) { return isDigit(C) || isLower(C) || C == '_'; }))
@@ -567,7 +566,7 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
         NewArch += ArchWithoutProfile.str();
       }
       return parseArchString(NewArch, EnableExperimentalExtension,
-                             ExperimentalExtensionVersionCheck, IgnoreUnknown);
+                             ExperimentalExtensionVersionCheck);
     }
   }
 
@@ -601,16 +600,8 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
     // Baseline is `i` or `e`
     if (auto E = getExtensionVersion(
             StringRef(&Baseline, 1), Exts, Major, Minor, ConsumeLength,
-            EnableExperimentalExtension, ExperimentalExtensionVersionCheck)) {
-      if (!IgnoreUnknown)
-        return std::move(E);
-      // If IgnoreUnknown, then ignore an unrecognised version of the baseline
-      // ISA and just use the default supported version.
-      consumeError(std::move(E));
-      auto Version = findDefaultVersion(StringRef(&Baseline, 1));
-      Major = Version->Major;
-      Minor = Version->Minor;
-    }
+            EnableExperimentalExtension, ExperimentalExtensionVersionCheck))
+      return std::move(E);
 
     // Postpone AddExtension until end of this function
     SeenExtMap[StringRef(&Baseline, 1).str()] = {Major, Minor};
@@ -677,11 +668,10 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
         Ext = StringRef();
 
         assert(!Type.empty() && "Empty type?");
-        if (!IgnoreUnknown && Name.size() == Type.size())
+        if (Name.size() == Type.size())
           return createStringError(errc::invalid_argument,
                                    Desc + " name missing after '" + Type + "'");
       } else {
-        // FIXME: Could it be ignored by IgnoreUnknown?
         return createStringError(errc::invalid_argument,
                                  "invalid standard user-level extension '" +
                                      Twine(Ext.front()) + "'");
@@ -690,27 +680,17 @@ RISCVISAInfo::parseArchString(StringRef Arch, bool EnableExperimentalExtension,
       unsigned Major, Minor, ConsumeLength;
       if (auto E = getExtensionVersion(Name, Vers, Major, Minor, ConsumeLength,
                                        EnableExperimentalExtension,
-                                       ExperimentalExtensionVersionCheck)) {
-        if (!IgnoreUnknown)
-          return E;
-
-        consumeError(std::move(E));
-        if (Name.size() == 1)
-          Ext = Ext.substr(ConsumeLength);
-        continue;
-      }
+                                       ExperimentalExtensionVersionCheck))
+        return E;
 
       if (Name.size() == 1)
         Ext = Ext.substr(ConsumeLength);
 
       // Check if duplicated extension.
-      if (!IgnoreUnknown && SeenExtMap.contains(Name.str()))
+      if (SeenExtMap.contains(Name.str()))
         return createStringError(errc::invalid_argument,
                                  "duplicated " + Desc + " '" + Name + "'");
 
-      if (IgnoreUnknown && !RISCVISAInfo::isSupportedExtension(Name))
-        continue;
-
       SeenExtMap[Name.str()] = {Major, Minor};
     } while (!Ext.empty());
   }
diff --git a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
index f944ac87b2cb0..eb5437b1ae3d4 100644
--- a/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
+++ b/llvm/unittests/TargetParser/RISCVISAInfoTest.cpp
@@ -341,25 +341,6 @@ TEST(ParseArchString, RejectsUnrecognizedExtensionNamesByDefault) {
       "unsupported non-standard user-level extension 'xmadeup'");
 }
 
-TEST(ParseArchString, IgnoresUnrecognizedExtensionNamesWithIgnoreUnknown) {
-  for (StringRef Input : {"rv32i_zmadeup", "rv64i_smadeup", "rv64i_xmadeup"}) {
-    auto MaybeISAInfo = RISCVISAInfo::parseArchString(Input, true, false, true);
-    ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
-    RISCVISAInfo &Info = **MaybeISAInfo;
-    const auto &Exts = Info.getExtensions();
-    EXPECT_EQ(Exts.size(), 1UL);
-    EXPECT_TRUE(Exts.at("i") == (RISCVISAUtils::ExtensionVersion{2, 1}));
-  }
-
-  // Checks that supported extensions aren't incorrectly ignored when a
-  // version is present (an early version of the patch had this mistake).
-  auto MaybeISAInfo =
-      RISCVISAInfo::parseArchString("rv32i_zbc1p0_xmadeup", true, false, true);
-  ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
-  const auto &Exts = (*MaybeISAInfo)->getExtensions();
-  EXPECT_TRUE(Exts.at("zbc") == (RISCVISAUtils::ExtensionVersion{1, 0}));
-}
-
 TEST(ParseArchString, AcceptsVersionInLongOrShortForm) {
   for (StringRef Input : {"rv64i2p1"}) {
     auto MaybeISAInfo = RISCVISAInfo::parseArchString(Input, true);
@@ -393,35 +374,6 @@ TEST(ParseArchString, RejectsUnrecognizedExtensionVersionsByDefault) {
             "unsupported version number 10.10 for extension 'zifencei'");
 }
 
-TEST(ParseArchString,
-     UsesDefaultVersionForUnrecognisedBaseISAVersionWithIgnoreUnknown) {
-  for (StringRef Input : {"rv32i0p1", "rv32i99p99", "rv64i0p1", "rv64i99p99"}) {
-    auto MaybeISAInfo = RISCVISAInfo::parseArchString(Input, true, false, true);
-    ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
-    const auto &Exts = (*MaybeISAInfo)->getExtensions();
-    EXPECT_EQ(Exts.size(), 1UL);
-    EXPECT_TRUE(Exts.at("i") == (RISCVISAUtils::ExtensionVersion{2, 1}));
-  }
-  for (StringRef Input : {"rv32e0p1", "rv32e99p99", "rv64e0p1", "rv64e99p99"}) {
-    auto MaybeISAInfo = RISCVISAInfo::parseArchString(Input, true, false, true);
-    ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
-    const auto &Exts = (*MaybeISAInfo)->getExtensions();
-    EXPECT_EQ(Exts.size(), 1UL);
-    EXPECT_TRUE(Exts.at("e") == (RISCVISAUtils::ExtensionVersion{2, 0}));
-  }
-}
-
-TEST(ParseArchString,
-     IgnoresExtensionsWithUnrecognizedVersionsWithIgnoreUnknown) {
-  for (StringRef Input : {"rv32im1p1", "rv64i_svnapot10p9", "rv32i_zicsr0p5"}) {
-    auto MaybeISAInfo = RISCVISAInfo::parseArchString(Input, true, false, true);
-    ASSERT_THAT_EXPECTED(MaybeISAInfo, Succeeded());
-    const auto &Exts = (*MaybeISAInfo)->getExtensions();
-    EXPECT_EQ(Exts.size(), 1UL);
-    EXPECT_TRUE(Exts.at("i") == (RISCVISAUtils::ExtensionVersion{2, 1}));
-  }
-}
-
 TEST(ParseArchString, AcceptsUnderscoreSplittingExtensions) {
   for (StringRef Input : {"rv32imafdczifencei", "rv32i_m_a_f_d_c_zifencei"}) {
     auto MaybeISAInfo = RISCVISAInfo::parseArchString(Input, true);

[jrtc27]

Isn't this functionality needed for things like llvm-objdump?..

[topperc]

Isn't this functionality needed for things like llvm-objdump?..

llvm-objdump uses parseNormalizedArchString

[wangpc-pp]

Isn't this functionality needed for things like llvm-objdump?..

Some digs into the history:

1. We added this IgnoreUnknown in https://reviews.llvm.org/D139553 to ignore unknown extension when parsing ELF attrs.
2. @asb added parseNormalizedArchString in https://reviews.llvm.org/D144353 to do parsing that ignores unknown extensions and removed the only use of IgnoreUnknown.

So it seems we don't need IgnoreUnknown any more.