[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/test/CodeGen/RISCV/riscv-rvv-intrinsics-generic/vadd.c:7-8
+// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature +d -target-feature +experimental-v \
+// RUN:   -target-feature +experimental-zfh -Werror -Wall -o - %s >/dev/null 2>%t
+// RUN: FileCheck --check-prefix=ASM --allow-empty %s <%t
+
----------------
This is poor style, but should be unnecessary per my comment below


================
Comment at: clang/test/CodeGen/RISCV/riscv-rvv-intrinsics-generic/vadd.c:10
+
+// ASM-NOT: warning
+#include <riscv_vector_generic.h>
----------------
Asm checks are discouraged in Clang. If you want to check for Clang warnings, use -verify, and in this case you want `// expected-no-diagnostics`.


================
Comment at: clang/test/CodeGen/RISCV/riscv-rvv-intrinsics-generic/vadd.c:38
+vint8m1_t test_vadd_vv_i8m1_m_vl(vbool8_t arg_1, vint8m1_t arg_2, vint8m1_t arg_3, vint8m1_t arg_4, size_t arg_5) {
+//
+  return vadd_m_vl(arg_1, arg_2, arg_3, arg_4, arg_5);
----------------
Delete


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:36
+public:
+  ELMULType() : ELMULType(0) {}
+  ELMULType(int ELMUL);
----------------
Surely being uninitialised is an error and we expect to always have it specified in the source?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:48-55
+  bool Float, Bool, Signed;
+  // Constant indices are "int", but have the constant expression.
+  bool Immediate;
+  bool Void;
+  // const qualifier.
+  bool Constant;
+  bool Pointer;
----------------
These are poor names; many of them don't sound like bools, and are some of them not mutually exclusive? If so, an enum would be better.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:55
+  bool Pointer;
+  bool SIZE_T, PtrDiff_T;
+  unsigned ElementBitwidth;
----------------
Capitalisation.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:56
+  bool SIZE_T, PtrDiff_T;
+  unsigned ElementBitwidth;
+  VScaleVal Vscale;
----------------
Capital W.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:57
+  unsigned ElementBitwidth;
+  VScaleVal Vscale;
+  bool Valid;
----------------
VScale. Or just drop the V and use Scale?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:87-95
+  bool isHalfVector() const {
+    return isVector() && Float && ElementBitwidth == 16;
+  }
+  bool isFloatVector() const {
+    return isVector() && Float && ElementBitwidth == 32;
+  }
+  bool isDoubleVector() const {
----------------
though at that point the wrappers are a bit unnecessary as they only seem to be used once.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:109
+  // Compute and record a string for legal type.
+  void compute_builtin_str();
+  // Compute and record a builtin RVV vector type string.
----------------
Camel-case?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:121
+
+enum class RISCV_Extension : uint8_t {
+  Basic = 0,
----------------
Using an `enum class` for a bitmask doesn't make much sense.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:294
+
+// type\lmul |1/8    |1/4      |1/2     |1       |2       |4        |8
+// --------  |------ |-------- |------- |------- |--------|-------- |--------
----------------
Please make sure there's at least 1 space around things


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:308
+    return false;
+  if (Float && ElementBitwidth == 8)
+    return false;
----------------
or 1? Clearer to move this into the switch below IMO.


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:315
+    // Check Vscale is 1,2,4,8,16,32,64
+    return (V <= 64 && countPopulation(V) == 1);
+  case 16:
----------------
isPowerOf2_32 from llvm/Support/MathExtras.h


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:380
+  }
+  if (!Float && !Bool) {
+    if (Signed)
----------------
Move into the !Float above


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:386
+  }
+  if (Immediate) {
+    assert(!Float && "fp immediates are not supported");
----------------
Move into the !Float above


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:512
+
+void RVVType::applyModifier(StringRef transformer) {
+  if (transformer.empty())
----------------
Capital T


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:693
+
+void RVVIntrinsic::emitFuncDelc(raw_ostream &OS, bool IsMangled) const {
+  // Index 0 is output type
----------------
Decl?


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:714
+  OS << "{\n";
+  OS << Twine("  return " + getName() + "(").str();
+  // Emit parameter variables
----------------
This is odd, why not `OS << "  return " + getName() + "(";` or `OS << "  return " << getName() << "(";`


================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:777
+  // Dump RVV int/float types.
+  for (char I : StringRef("csil"))
+    for (int ELMUL : ELMULs) {
----------------
Personally not a fan of omitting the curly braces in cases like this, it's non-obvious and more error-prone than when you don't have nesting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95016