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

Tue Jan 19 20:14:08 PST 2021

jrtc27 added inline comments.

================
Comment at: clang/include/clang/Basic/riscv_vector.td:9
+//
+// This file defines builtins for RISCV-V V-extension. See:
+//
----------------
"the"

================
Comment at: clang/include/clang/Basic/riscv_vector.td:23
+// The elements of this collection are defined by an instantiation process the
+// range of which is specified by cross product of the LMUL attribute and every
+// element in the attribute TypeRange. By default builtins have LMUL = [1, 2,
----------------
Missing "the"

================
Comment at: clang/include/clang/Basic/riscv_vector.td:25
+// element in the attribute TypeRange. By default builtins have LMUL = [1, 2,
+// 4, 8, -2, -4, -8] so the process is repeated 7 times. A negative LMUL -x
+// in that list is used to represent the fractional LMUL 1/x.
----------------
Why not just make it an exponent instead? Then that falls out naturally rather than having this weird split interpretation.

================
Comment at: clang/include/clang/Basic/riscv_vector.td:42
+//   i: int (32-bit)
+//   l: long (64-bit)
+//   h: half (16-bit)
----------------
If this is a C long, that's not true for RV32. If it's not a C long, call all these something else.

Though either way it'd be better to use the architectural names or LLVM-ish names for these things rather than something approximating the C language-level names.

================
Comment at: clang/include/clang/Basic/riscv_vector.td:47
+//
+// This way, given an LMUL, a record with a TypeRange "sil" will cause the
+// definition of 3 builtins. Each type "t" in the TypeRange (in this example
----------------
Strings that are really lists is a bit gross. I'd use TableGen sequences of LLVM types.

================
Comment at: clang/include/clang/Basic/riscv_vector.td:56
+//
+//   e: type of "t" as is (identity)
+//   v: computes a vector type whose element type is "t" for the current LMUL
----------------
Do we really need to invent an esoteric DSL?

================
Comment at: clang/include/clang/Basic/riscv_vector.td:66
+//      element type which is bool
+//   0: void type, ignores "t"
+//   z: size_t, ignores "t"
----------------
Then why aren't these just base types? We don't have to follow the brain-dead nature of printf.

================
Comment at: clang/include/clang/Basic/riscv_vector.td:116
+class RVVBuiltin<string suffix, string prototype,
+                 string type_range, string managed_suffix = "">
+{
----------------
Curly braces almost always go on the same line in .td files.

================
Comment at: clang/include/clang/Basic/riscv_vector.td:118
+{
+  // Base name that will be prepended __builtin_rvv_ and appended the computed
+  // Suffix.
----------------
Grammar

================
Comment at: clang/include/clang/Basic/riscv_vector.td:123
+  // If not empty, each instantiated builtin will have this appended after an
+  // underscore (_). Suffix is instantiated like Prototype.
+  string Suffix = suffix;
----------------
Don't need to repeat the field name.

================
Comment at: clang/include/clang/Basic/riscv_vector.td:130
+
+  // For each type described in TypeRange we instantiate this Prototype.
+  string Prototype = prototype;
----------------
Could do with a better explanation (you don't instantiate the prototype, you use the prototype to instantiate a specific element of the set of builtins being defined).

================
Comment at: clang/include/clang/Basic/riscv_vector.td:139
+
+  // If HasMask == 1, this flag states that this builtin has a first merge
+  // operand.
----------------
What's a "first merge" operand? Is the first important? Because the field itself is called HasMergeOperand, no mention of first. If merge operands are always first then rephrase the comment to not make it sound like coming first is special.

================
Comment at: clang/include/clang/Basic/riscv_vector.td:146
+
+  // This builtin supprot function overloading and has a mangled name
+  bit HasGeneric = 1;
----------------
support

================
Comment at: clang/include/clang/Basic/riscv_vector.td:149
+
+  // Reads or writes "memory".
+  bit HasSideEffects = 0;
----------------
If it's solely memory then call the field something to do with that. If there can be other side effects then don't make the comment so specific.

================
Comment at: clang/include/clang/Basic/riscv_vector.td:152
+
+  // This builtin is valid for the given LMUL.
+  list<int> LMUL = [1, 2, 4, 8, -2, -4, -8];
----------------
Plural

================
Comment at: clang/include/clang/Basic/riscv_vector.td:155-161
+  // emit the automatic clang codegen. It describes
+  // what types we have to use to obtain the specific LLVM intrinsic.
+  //
+  // -1 means the return type,
+  // otherwise, k >= 0 meaning the k-th operand (counting from zero) of the
+  // codegen'd parameter of the unmasked version. k can't be the mask operand's
+  // position.
----------------
This needs proper formatting and capitalisation.

================
Comment at: clang/include/clang/Basic/riscv_vector.td:167
+  string IRName = NAME;
+  string IRNameMask = NAME # "_mask";
+}
----------------
Is this ever not `IRName # "_mask"`? The only overrider of the fields still preserves that.

================
Comment at: clang/include/clang/Basic/riscv_vector.td:188
+    {
+      let Name = NAME # "_" # !head(s_p) in
+      {
----------------
Does it not work if you use the suffix for the inner record name?

================
Comment at: clang/include/clang/Basic/riscv_vector.td:190-191
+      {
+        defvar suffix =  !head(!tail(s_p));
+        defvar prototype =  !head(!tail(!tail(s_p)));
+
----------------
Whitespace

================
Comment at: clang/include/clang/Basic/riscv_vector.td:199
+
+// `All` marco should be defined in all gen-riscv-* target except the
+// gen-riscv-vector-test.
----------------
All -> ALL
marco -> macro
target -> targets
the -> (delete)

================
Comment at: clang/include/clang/Basic/riscv_vector.td:204
+// op_list in gen-riscv-v-tests.sh.
+#ifdef ALL
+#define VADD
----------------
Probably nicer to have a notion of a builtin group and allow the records to be filtered (with the default being to process all of them). Depending on what's still to come you could even use NAME automatically as the group for RVVBinBuiltinSet's RVV(Bin)Builtins given that's always the lowercase version of the macro guarding the definitions at the moment.

================
Comment at: clang/include/clang/Basic/riscv_vector.td:219
+
+#ifdef VFADD
+defm vfadd : RVVBinBuiltinSet<"vfadd", "fd",
----------------
Does this one get a comment heading?

================
Comment at: clang/lib/Headers/CMakeLists.txt:206
+clang_generate_header(-gen-arm-neon
+                      SOURCE arm_neon.td
+		      OUTPUT arm_neon.h)
----------------
Indentation is messed up for all of these

================
Comment at: clang/utils/TableGen/CMakeLists.txt:22
   SveEmitter.cpp
+  RISCVVEmitter.cpp
   TableGen.cpp
----------------
Sort

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:124
+
+// TODO rafactor Intrinsic class design after support all intrinsic combination.
+class Intrinsic {
----------------
craig.topper wrote:
> rafactor->refactor
refactor

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:125
+// TODO rafactor Intrinsic class design after support all intrinsic combination.
+class Intrinsic {
+  enum class Extension : uint8_t {
----------------
This seems like it's asking for collisions with llvm::Intrinsic

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:240
+  if (LMUL < 0)
+    LMul = 1.f / static_cast<float>(ULMul);
+  else
----------------
You don't want floats.

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:276
+
+//        | lmul=⅛ | lmul=¼  | lmul=½  | lmul=1  | lmul=2  | lmul=4   | lmul=8
+// ------ | ------ | --------| ------- | ------- | --------| -------- | --------
----------------
No unicode please

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:406
+
+void RVVType::compute_str() {
+  assert(isValid() && "RVVType is invalid");
----------------
This is a bad name given you have the other compute_foo_str functions above. How does this differ?

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:431
+    S += "uint";
+  /* vbool1_t = MVT::nxv64i1;
+   * vbool2_t = MVT::nxv32i1;
----------------
Is this just examples or left-over debugging? If it's meant to be there then use words to explain it (and use C++-style comments), otherwise delete it.

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:527
+  case 'm':
+    /* vbool1_t = MVT::nxv64i1;
+     * vbool2_t = MVT::nxv32i1;
----------------
Ditto

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:661
+    ExtVector.emplace_back("__riscv_flen == 32");
+    ExtVector.emplace_back("__riscv_f != 0");
+    ExtVector.emplace_back("__riscv_flen == 64");
----------------
defined(..) not != 0

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:708
+  } else {
+    // IntrinsicTypes is ummasked version index
+    // we need to update IntrinsicTypes because it does not count the additional
----------------
Missing a "the" and not full sentences.

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:723
+    // The order of intrinsic operands is (maskedoff, op1, op2, mask, vl)
+    // or
+    // The order of operands is (mask, op1, op2, vl).
----------------
When is it each of them?

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:732
+std::string Intrinsic::getFuncDelc(Twine FuncName) const {
+  // index 0 is output type
+  std::string S =
----------------
Capitalise

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:740
+  };
+  // append function arguments string
+  if (Types.size() > 1) {
----------------
Ditto

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:755
+  S += Twine("  return " + CalleeName + "(").str();
+  // append parameter variables
+  if (Types.size() > 1) {
----------------
Ditto

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:795
+
+  // TODO: Which C type should be used for float16_t need to update psabi spec
+  // first.
----------------
Grammar (and it's psABI)

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:805
+
+  // parse records
+  SmallVector<std::unique_ptr<Intrinsic>, 512> Defs;
----------------
Capitalise

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:841
+
+  // D imply F
+  OS << "#if (__riscv_flen == 32) || (__riscv_f != 0) || (__riscv_flen == 64) "
----------------
implies

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:842
+  // D imply F
+  OS << "#if (__riscv_flen == 32) || (__riscv_f != 0) || (__riscv_flen == 64) "
+        "|| (__riscv_d != 0)\n";
----------------
defined(..) not != 0

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:922
+
+  // Same intrinsic name have the same switch body
+  llvm::StringMap<SmallVector<std::unique_ptr<Intrinsic>, 128>> DefsSet;
----------------
Plural

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:944
+  for (auto &Def : Defs) {
+    // Some intrinsis have no generic functions
+    if (!Def->hasGeneric() && IsGeneric)
----------------
intrinsics

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:952
+    return;
+  OS << "// RUN: %clang_cc1 -triple riscv64 -target-feature +f -target-feature "
+        "+d -target-feature +experimental-v \\\n";
----------------
RV32 then RV64, not the other way round.

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1008
+  }
+  // if HasVL, append 'z' to last operand
+  if (HasVL)
----------------
Capitalise

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1014
+  if (HasMask) {
+    // if HasMask, insert 'm' as first input operand
+    ProtoMaskSeq.insert(ProtoMaskSeq.begin() + 1, "m");
----------------
Ditto

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1016
+    ProtoMaskSeq.insert(ProtoMaskSeq.begin() + 1, "m");
+    // if HasMergeOperand, insert result type as second input operand
+    if (HasMergeOperand)
----------------
Ditto

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1021
+
+  // Create Intrinsics for each type and LMUL
+  for (char I : TypeRange) {
----------------
Why does Intrinsics get a capital letter all of a sudden?

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1025
+      Optional<RVVTypes> Types = computeTypes(I, LMUL, ProtoSeq);
+      // Ignored to create new intrinsic if there is any illegal type.
+      if (!Types.hasValue())
----------------
are any illegal types

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1037
+          Name, SuffixStr, MSuffixStr, IRName, HasSideEffects,
+          /*IsMask*/ false, /*HasMergeOperand*/ false, HasVL, HasGeneric,
+          Types.getValue(), IntrinsicTypes));
----------------
`/*IsMask=*/` with no space after is the correct style (repeated several times)

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1060
+    }
+    // record legal type index
+    Types.push_back(T.getValue());
----------------
Capitalise

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1069
+  TypeString Idx = Twine(BT + utostr(LMUL) + Proto).str();
+  // search first
+  if (LegalTypes.count(Idx)) {
----------------
Ditto

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1075
+  }
+  // compute type and record the result
+  auto T = std::make_shared<RVVType>(BT, LMUL, Proto);
----------------
Ditto

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1078
+  if (T->isValid()) {
+    // record legal type index and value
+    LegalTypes.insert({Idx, T});
----------------
Ditto

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1082
+  }
+  // record illegal type index
+  IllegalTypes.insert(Idx);
----------------
Ditto

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1091
+
+  // collect same extensions in one set.
+  DenseMap<uint8_t, SmallVector<std::unique_ptr<Intrinsic>, 256>> DefsSet;
----------------
Ditto (and not a sentence; either make it one or drop the full stop)

================
Comment at: clang/utils/TableGen/RISCVVEmitter.cpp:1100
+        Intrinsic::getExtStrings(KV.getFirst());
+    // dump arch predecessor definitions
+    if (ExtStrings.size()) {
----------------
Ditto

================
Comment at: clang/utils/TestUtils/gen-riscv-v-tests.sh:1
+#!/bin/bash
+#
----------------
Scripts are generally written in Python, otherwise they start off as simple shell scripts and grow into a mess. As a bonus you can then just call update_cc_test_checks as a library rather than needing to fork+exec.

================
Comment at: clang/utils/TestUtils/gen-riscv-v-tests.sh:3
+#
+# Generate riscv-v vector intrinsic tests in clang/test/CodeGen/RISCV/riscv-rvv-intrinsics
+# and clang/test/CodeGen/RISCV/riscv-rvv-intrinsics-generic
----------------
RISC-V V (or just drop the V, it's implied by vector)

================
Comment at: clang/utils/TestUtils/gen-riscv-v-tests.sh:17
+
+src_path="$(dirname $(realpath $0))/../../../"
+bin_path=$1
----------------
realpath is not portable (specifically it doesn't exist on macOS, only if you install GNU coreutils). More reasons to write in Python.

================
Comment at: clang/utils/TestUtils/gen-riscv-v-tests.sh:20
+
+gen_tests(){
+# op_list have marco name used in riscv_vector.td
----------------
Space?

================
Comment at: clang/utils/TestUtils/gen-riscv-v-tests.sh:26
+  local path="${src_path}/clang/test/CodeGen/RISCV/riscv-rvv-intrinsics${suffix}"
+  if [[ ! -d $path ]]; then
+    mkdir $path -p
----------------
Unnecessary bashism

================
Comment at: llvm/docs/CommandGuide/tblgen.rst:562
+
+  Generate RISCV Vector tests for Clang.
+
----------------
Generating tests scares me, both from an "oops you accidentally lost tests" perspective and from an "I need to fix the tests but the generator is insufficient" perspective. But I see NEON already does this, and I guess the file is committed so you'll (hopefully) spot disappearing tests.

Regardless, it's RISC-V not RISCV.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95016