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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:50
+    Boolean,
+    SignInteger,
+    UnsignedInteger,
----------------
Signed


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:58-62
+  bool IsPointer = false;
+  // IsConstant indices are "int", but have the constant expression.
+  bool IsImmediate = false;
+  // const qualifier.
+  bool IsConstant = false;
----------------
This isn't expressive enough for the grammar you defined. `PCPCec` is supposed to give `const i8 * const i8 *`, whereas this will interpret it as `const i8 *`. Given such types are presumably not needed you need to tighten the rules of your grammar.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:127
+
+enum RISCV_Extension : uint8_t {
+  Basic = 0,
----------------
No underscores in names


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:173
+  StringRef getIRName() const { return IRName; }
+  uint8_t getRISCV_Extensions() const { return RISCV_Extensions; }
+
----------------
No underscores in names


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:354-356
+  }
+
+  switch (ScalarType) {
----------------
Combine the two switch statements


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:419
+  ClangBuiltinStr = "__rvv_";
+  if (isBoolean()) {
+    ClangBuiltinStr += "bool" + utostr(64 / Scale.getValue()) + "_t";
----------------
Combine this with the switch


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:441
+  assert(isValid() && "RVVType is invalid");
+  assert(ScalarType != ScalarTypeKind::Invalid && "ScalarType is invalid");
+  switch (ScalarType) {
----------------
Combine this with the switch


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:452-467
+  default:
+    break;
+  }
+
+  if (IsConstant)
+    Str += "const ";
+
----------------
This should be able to be tidied up so there's only one switch


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:627
+    case 'W':
+      assert(isVector() && "'W' type transformer cannot be used on vectors");
+      ElementBitwidth *= 2;
----------------
This looks wrong


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:259-260
+
+LMULType &LMULType::operator*=(unsigned RHS) {
+  this->Log2LMUL = this->Log2LMUL + RHS;
+  return *this;
----------------
khchen wrote:
> craig.topper wrote:
> > jrtc27 wrote:
> > > That's not how multiplication works. This is exponentiation. Multiplication would be `Log2LMul + log2(RHS)`. Please don't abuse operators like this.
> > This seems like it must be broken, but since we don't do widening or narrowing in this patch we didn't notice?
> Yes, thanks for point out. In my original plan is fixing that in followup patches. 
> I also add more bug fixes into this patch.
Probably worth adding an an `assert(isPowerOf2_32(RHS));` too


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95016