[PATCH] D115668: [RISCV] Add a table for extension implications.

[Yueh-Ting Chen via Phabricator via llvm-commits]

eopXD added a comment.

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



================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:722
 
-  if (HasV) {
-    auto Version = findDefaultVersion("zvlsseg");
-    addExtension("zvlsseg", Version->Major, Version->Minor);
-  }
-
-  if (HasZfh) {
-    auto Version = findDefaultVersion("zfhmin");
-    addExtension("zfhmin", Version->Major, Version->Minor);
+  for (auto &Ext : Exts) {
+    auto I = llvm::lower_bound(ImpliedExts, Ext.first,
----------------
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?


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:725
+                               [](const ImpliedExtsEntry &Entry,
+                                  StringRef Ext) { return Entry.Name < Ext; });
+    if (I != std::end(ImpliedExts) && I->Name == Ext.first) {
----------------
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},
};
```



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115668/new/

https://reviews.llvm.org/D115668