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