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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits