[PATCH] D108694: [RISCV] Add the zvl extension according to the v1.0 spec

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:714
 void RISCVISAInfo::updateImplication() {
+  const StringMap<std::vector<StringRef>> Implications = {
+      {"v", {"zvlsseg", "zvl128b"}},
----------------
I think I'd like to see this as a static data structure rather than building a StringMap on the fly.

Maybe like

```
static const char *zvl64bimplied[] = { "zvl32b" };
static const char *zvl128bimplied[] = { "zvl64b" };
...

struct ImpliedEntry = {
  StringLiteral Name;
  ArrayRef<const char*> ImpliedExtensions;
};

static constexpr ImpliedEntry ImpliedTable[] = {
  { "zvl64b", zvl64bimplied },
  { "zvl128b", zvl128implied },
  ...
};
```

You can then use std::lower_bound to search the ImpliedTable to find the correct row of ImpliedTable. I haven't tested this. Maybe I'll put up a patch on the existing V implications as a proof of concept.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:782
+    if (IsZvlExt) {
+      ExtName.consume_back("b");
+      unsigned ZvlLen;
----------------
I think we should check the return value from consume_back and getAsInteger to make sure we really parsed what we think we parsed. That will prevent surprises if a new extension comes along that also starts with "zvl"


================
Comment at: llvm/lib/Target/RISCV/RISCVSubtarget.cpp:136
   assert((RVVVectorBitsMin == 0 ||
-          (RVVVectorBitsMin >= 128 && RVVVectorBitsMax <= 65536 &&
+          (RVVVectorBitsMin >= 64 && RVVVectorBitsMax <= 65536 &&
            isPowerOf2_32(RVVVectorBitsMin))) &&
----------------
This needs the same FIXME as above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108694