[PATCH] D124730: [RISCV][NFC] Refactor RISC-V vector intrinsic utils.

[Zakk Chen via Phabricator via cfe-commits]

khchen added a comment.
Herald added a subscriber: shiva0217.

Thanks for refactoring!



================
Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:55
+// basic vector type, used to compute type info of arguments.
+enum class PrimitiveType : uint8_t {
+  Invalid,
----------------
I think vector is not a primitive type in common sense, is it?
why Widening2XVector, Widening4XVector, Widening8XVector and MaskVector is not part of VectorTypeModifier?

Sorry, I'm confused and maybe forget something.


================
Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:85
+// TypeProfile is used to compute type info of arguments or return value.
+struct TypeProfile {
+  constexpr TypeProfile() = default;
----------------
I think we need to update the comment in riscv_vector.td to sync the word "TypeProfile", I feel the new word `TypeProfile` is similar to `modifier` or `prototype`, is it?

```
The C/C++ prototype of the builtin is defined by the Prototype attribute.
Prototype is a non-empty sequence of type transformers, the first of which
is the return type of the builtin and the rest are the parameters of the
builtin, in order. For instance if Prototype is "wvv" and TypeRange is "si"
a first builtin will have type
```

we call it  `modifier` or `prototype` is because those words are coming from clang intrinsic definition and other target.

https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Builtins.def#L52
https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/arm_sve.td#L58

personally I think consistent naming maybe better than creating a new word, what do you think?


BTW, I think having this new class is really good idea for refactoring!


================
Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:90
+      : PT(static_cast<uint8_t>(PT)), TM(static_cast<uint8_t>(TM)) {}
+  constexpr TypeProfile(uint8_t PT, uint8_t VTM, uint8_t TM)
+      : PT(PT), VTM(VTM), TM(TM) {}
----------------
If we allow parameter could `uint8_t`, why other constructors not follow the same rule?


================
Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:97
+
+  std::string IndexStr() const {
+    return std::to_string(PT) + "_" + std::to_string(VTM) + "_" +
----------------
What's purpose of this function, translate TypeProfile to the Proto string?


================
Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:249
+  /// and LMUL with type transformers). It also record result of type in legal
+  /// or illegal set to avoid compute the  same config again. The result maybe
+  /// have illegal RVVType.
----------------
additional space


================
Comment at: clang/lib/Support/RISCVVIntrinsicUtils.cpp:767
+
+void RVVType::applyFixedLog2LMUL(int Log2LMUL, bool LargerThan) {
+  if (LargerThan) {
----------------
In riscv_vector.td is said smaller or larger, I feel little confusing here when it call LagerThan. maybe have more comment like `The result of modified type should be smaller than giving type` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124730