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

[Kito Cheng via Phabricator via cfe-commits]

kito-cheng added inline comments.


================
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,
----------------
khchen wrote:
> 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.
> I think vector is not a primitive type in common sense, is it?
It's called `primitive type transformer` on the comment[1], so that's why I use PrimitiveType here, but I admit that's kind of confusing, maybe BaseTypeModifier?

> Widening2XVector, Widening4XVector, Widening8XVector and MaskVector is not part of VectorTypeModifier?

Good point, Moved to VectorTypeModifier!

[1] https://github.com/llvm/llvm-project/blob/main/clang/lib/Support/RISCVVIntrinsicUtils.cpp#L366


================
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;
----------------
khchen wrote:
> 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!
Renamed to `PrototypeDescriptor`, that should be better I guess :)


================
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) {}
----------------
khchen wrote:
> If we allow parameter could `uint8_t`, why other constructors not follow the same rule?
Keep only two version of constructor, one for all uint8_t and one for all enum.

All enum one used for checking type safe as possilbe.
And uint8_t version used for low-level construct (used in https://reviews.llvm.org/D111617)


================
Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:97
+
+  std::string IndexStr() const {
+    return std::to_string(PT) + "_" + std::to_string(VTM) + "_" +
----------------
khchen wrote:
> What's purpose of this function, translate TypeProfile to the Proto string?
Used for caching the `RVVType`, but I realized we could use integer(`uint64_t`) as hash value, use new hash scheme in new version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124730