[PATCH] D145088: [RISCV] Add attribute(riscv_rvv_vector_bits(N)) based on AArch64 arm_sve_vector_bits.

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

The CodeGen change looks fine.  I'm surprised you didn't need any code in argument/parameter/call/return emission to do the actual fixed<->scalable coercion; do we already have that for other reasons?



================
Comment at: clang/include/clang/Basic/AttrDocs.td:2332
+
+  #if __RISCV_RVV_VLEN_BITS==512
+  typedef vint8m1_t fixed_vint8m1_t __attribute__((riscv_rvv_vector_bits(512)));
----------------
This probably needs a `defined(__RISCV_RVV_VLEN_BITS)` clause, right?  Because the compiler doesn't actually define this macro unless `-mrvv-vector-bits` is given.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:2337
+Creates a type ``fixed_vint8m1_t_t`` that is a fixed-length variant of
+``vint8m1_t`` that contains exactly 512-bits. Unlike ``vint8m1_t``, this type
+can be used in globals, structs, unions, and arrays, all of which are
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:2345
+is enabled under the ``-mrvv-vector-bits`` flag. ``__RISCV_RVV_VLEN_BITS`` can
+only be a power of 2 between 64 and 65536.
+
----------------
This doesn't describe the actual behavior of the compiler, which is that it's *ill-formed* to use this attribute except when providing the same value to `-mrvv-vector-bits`.

Also, this feels like an awkward attempt to also document the `__RISCV_RVV_VLEN_BITS` macro, which probably ought to be primarily documented in the command line argument reference for `-mrvv-vector-bits`.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:2347
+
+Only `*m1_t`(LMUL=1) types are supported at this time.
+}];
----------------



================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:207
+    Builder.defineMacro("__RISCV_RVV_VLEN_BITS",
+                        Twine(VScale->first * llvm::RISCV::RVVBitsPerBlock));
 }
----------------
Is this macro name coming from somewhere specifically?  Because it doesn't match the normal scheme for RISC-V target macros, which are all lowercase, and it doesn't match the name of the command line argument it reflects.

Also, why is the computation of this thing so complicated when the command-line argument is basically a single number?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145088