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

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
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)));
----------------
rjmccall wrote:
> 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.
I guess so. I copied the documentation from the SVE attribute and modified it to RISC-V.


================
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.
+
----------------
rjmccall wrote:
> 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`.
> 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`.

I think that means the SVE doc is also incorrect?

> 
> 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`.

Ok I'll move it there.


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:207
+    Builder.defineMacro("__RISCV_RVV_VLEN_BITS",
+                        Twine(VScale->first * llvm::RISCV::RVVBitsPerBlock));
 }
----------------
rjmccall wrote:
> 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?
> 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.

I made it up. I'll reconsider it.

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

The command line is converted to `-mvscale-min=` and `-mvscale-max=` options just like SVE. We divide by `llvm::RISCV::RVVBitsPerBlock` where SVE divides by 128.

RISC-V does have a concept of minimum vector length through -march already which is checked by getVScaleRange to deal with any disagreement. There's a special value `-mriscv-rvv-vector-bits=zvl` to use the minimum value from -march without needing to repeat the value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145088