[RISCV] Add a table for extension implications.

[craig.topper]

Summary

This a proof of concept for a suggestion I proposed in D108694.

[eopXD]

LGTM. I will apply it onto D108694.

[eopXD]

llvm/lib/Support/RISCVISAInfo.cpp

698

Will the variables with underscore violate the naming conventions in LLVM?

[craig.topper]

llvm/lib/Support/RISCVISAInfo.cpp

698

Yeah. Maybe I should use ImpliedExtsV and ImpliedExtsZfh so that a capital letter is against the ImpliedExts part for readability?

[eopXD]

Sorry I accepted this without giving a closer look.
Some comments added :-)

llvm/lib/Support/RISCVISAInfo.cpp

728–736

I think this won't cover recursive dependencies?
Additionally, Calling addExtension will invalidate the range of Exts.
Maybe the "work list way" I did on the zvl patch is more correct than this single for loop?

731

Does this mean the ImpliedExts have to have extensions sorted in lexicological order? If so adding the zvl extension would look like: (I think it looks strange in my opinion, but no big problem though

static constexpr ImpliedExtsEntry ImpliedExts[] = {
    {"v", ImpliedExtsV},
    {"zfh", ImpliedExtsZfh},
    {"zvl64b", ImpliedExtsZvl64b},
    {"zvl1024b", ImpliedExtsZvl1024b},
    {"zvl128b", ImpliedExtsZvl128b},
    {"zvl16384b", ImpliedExtsZvl16384b},
    {"zvl2048b", ImpliedExtsZvl2048b},
    {"zvl256b", ImpliedExtsZvl256b},
    {"zvl32768b", ImpliedExtsZvl32768b},
    {"zvl4096b", ImpliedExtsZvl4096b},
    {"zvl512b", ImpliedExtsZvl512b},
    {"zvl65536b", ImpliedExtsZvl65536b},
    {"zvl8192b", ImpliedExtsZvl8192b},
};

[craig.topper]

llvm/lib/Support/RISCVISAInfo.cpp

731

Yes. It is unfortunate that they have to be ordered like that. I should also add a check to verify the table is sorted to prevent mistakes.

[craig.topper]

Add is_sorted check. Add operator< to the ImpliedFeatures struct
Remove underscore from array names.

[craig.topper]

clang-format

[eopXD]

Looks good. Thank you for the patch!

[eopXD]

llvm/lib/Support/RISCVISAInfo.cpp

728–736

The comment don't need to be addressed. It is a problem D108694 will deal with,

[mehdi_amini]

This does not build with gcc5 apparently: https://lab.llvm.org/staging/#/builders/190/builds/2632

llvm/lib/Support/RISCVISAInfo.cpp:715:1: error: could not convert '(const char*)"v"' from 'const char*' to 'llvm::StringLiteral'
llvm/lib/Support/RISCVISAInfo.cpp:715:1: error: could not convert '(const char**)(& ImpliedExtsV)' from 'const char**' to 'llvm::ArrayRef<const char*>'
llvm/lib/Support/RISCVISAInfo.cpp:715:1: error: could not convert '(const char*)"zfh"' from 'const char*' to 'llvm::StringLiteral'
llvm/lib/Support/RISCVISAInfo.cpp:715:1: error: could not convert '(const char**)(& ImpliedExtsZfh)' from 'const char**' to 'llvm::ArrayRef<const char*>'

[craig.topper]

In D115668#3194038, @mehdi_amini wrote:

This does not build with gcc5 apparently: https://lab.llvm.org/staging/#/builders/190/builds/2632

llvm/lib/Support/RISCVISAInfo.cpp:715:1: error: could not convert '(const char*)"v"' from 'const char*' to 'llvm::StringLiteral'
llvm/lib/Support/RISCVISAInfo.cpp:715:1: error: could not convert '(const char**)(& ImpliedExtsV)' from 'const char**' to 'llvm::ArrayRef<const char*>'
llvm/lib/Support/RISCVISAInfo.cpp:715:1: error: could not convert '(const char*)"zfh"' from 'const char*' to 'llvm::StringLiteral'
llvm/lib/Support/RISCVISAInfo.cpp:715:1: error: could not convert '(const char**)(& ImpliedExtsZfh)' from 'const char**' to 'llvm::ArrayRef<const char*>'

Thanks. I seem to remember something about gcc 5.x and constexprs before. I’ll take a look

[craig.topper]

In D115668#3194041, @craig.topper wrote:

In D115668#3194038, @mehdi_amini wrote:

This does not build with gcc5 apparently: https://lab.llvm.org/staging/#/builders/190/builds/2632

llvm/lib/Support/RISCVISAInfo.cpp:715:1: error: could not convert '(const char*)"v"' from 'const char*' to 'llvm::StringLiteral'
llvm/lib/Support/RISCVISAInfo.cpp:715:1: error: could not convert '(const char**)(& ImpliedExtsV)' from 'const char**' to 'llvm::ArrayRef<const char*>'
llvm/lib/Support/RISCVISAInfo.cpp:715:1: error: could not convert '(const char*)"zfh"' from 'const char*' to 'llvm::StringLiteral'
llvm/lib/Support/RISCVISAInfo.cpp:715:1: error: could not convert '(const char**)(& ImpliedExtsZfh)' from 'const char**' to 'llvm::ArrayRef<const char*>'

Thanks. I seem to remember something about gcc 5.x and constexprs before. I’ll take a look

We might just need more curly braces.

llvm/lib/Support/RISCVISAInfo.cpp

 //===-- RISCVISAInfo.cpp - RISCV Arch String Parser --------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
Error RISCVISAInfo::checkDependency() {
 
   // Additional dependency checks.
   // TODO: The 'q' extension requires rv64.
   // TODO: It is illegal to specify 'e' extensions with 'f' and 'd'.
 
   return Error::success();
 }
 
+static const char *ImpliedExtsV[] = {"zvlsseg"};
+static const char *ImpliedExtsZfh[] = {"zfhmin"};
+
+struct ImpliedExtsEntry {
+  StringLiteral Name;
+  ArrayRef<const char *> Exts;
+
+  bool operator<(const ImpliedExtsEntry &Other) const {
+    return Name < Other.Name;
+  }
+
+  bool operator<(StringRef Other) const { return Name < Other; }
+};
+
+static constexpr ImpliedExtsEntry ImpliedExts[] = {
+    {"v", ImpliedExtsV},
+    {"zfh", ImpliedExtsZfh},
+};
+
 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);
+  assert(llvm::is_sorted(ImpliedExts) && "Table not sorted by Name");
+  for (auto &Ext : Exts) {
+    auto I = llvm::lower_bound(ImpliedExts, Ext.first);
+    if (I != std::end(ImpliedExts) && I->Name == Ext.first) {
+      for (auto &ImpliedExt : I->Exts) {
+        auto Version = findDefaultVersion(ImpliedExt);
+        addExtension(ImpliedExt, 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.
   if (Exts.count("d"))
     FLen = 64;