[PATCH] D95016: [Clang][RISCV] Add custom TableGen backend for riscv-vector intrinsics.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: clang/include/clang/Basic/riscv_vector.td:148
+  // If HasMask, this flag states that this builtin has a maskedoff operand. It
+  // is always the first operand.
+  bit HasMaskedOffOperand = true;
----------------
Isn't mask the first operand maskedoff the second operand?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:157
+  ArrayRef<int64_t> getIntrinsicTypes() const { return IntrinsicTypes; }
+  std::string getIRName() const { return IRName; }
+  uint8_t getRISCV_Extensions() const { return RISCV_Extensions; }
----------------
Return by const reference or use StringRef.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:175
+
+using TypeString = std::string;
+class RVVEmitter {
----------------
Why not just use std::string in the one place this is used?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:862
+
+  // The same intrinsic name has the same switch body.
+  llvm::StringMap<SmallVector<std::unique_ptr<RVVIntrinsic>, 128>> DefsSet;
----------------
Might be better to just sort the vector by the IR name and then walk the vector looking for the boundaries where the name changes from the previous iteration.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:899
+    // (operand) in ProtoSeq. ProtoSeq[0] is output operand.
+    SmallVector<std::string, 8> ProtoSeq;
+    const StringRef Primaries("evwqom0ztc");
----------------
I think this is something like

```
while (!Prototypes.empty()) {
 auto Idx = Prototypes.find_first_of(Primaries);
 assert(Idx != StringRef::npos);
 ProtoSeq.push_back(Prototypes.slice(0, Idx+1).str());
 Prototypes = Prototypes.drop_front(Idx+1);
}
```

Which might be easier to understand.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:970
+  if (It != LegalTypes.end())
+    return Optional<RVVTypePtr>(&(It->second));
+  if (IllegalTypes.count(Idx))
----------------
Why does this need to explicitly create an Optional? Shouldn't we just be able to return &(It->second)?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:990
+  // Collect the same extension intrinsic in the one set for arch guard marco.
+  DenseMap<uint8_t, SmallVector<std::unique_ptr<RVVIntrinsic>, 256>> DefsSet;
+  for (auto &def : Defs) {
----------------
Could we maybe just sort the original vector by extension and just loop over it. Keep track of the extensions from previous iteration and emit the guard when this iteration doesn't match the previous iteration.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1012
+
+SmallVector<std::string> RVVEmitter::getExtStrings(uint8_t Extents) {
+  if (Extents == 0)
----------------
This is only called in one place which immediate loops over and prints the contents. Could we just pass the raw_ostream in here and do the printing directly?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1015
+    return {};
+  SmallVector<std::string> ExtVector;
+  // D implies F
----------------
Even if we don't print directly. This can be a vector of StringRef. These are all string literals.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1016
+  SmallVector<std::string> ExtVector;
+  // D implies F
+  if (Extents & RISCV_Extension::F) {
----------------
I don't understand this. It says D implies F but we're checking for F. So that seems like F implies D.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95016