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

> You mean RISC-V specific code or generic code? If generic, I assume we got it from SVE's earlier implementation.

Ah, if SVE has a similar feature then that makes sense.



================
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)));
----------------
craig.topper wrote:
> 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.
Ah, I see.  Yeah, it's probably wrong there, too.


================
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.
+
----------------
craig.topper wrote:
> 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.
> I think that means the SVE doc is also incorrect?

Yeah.


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:207
+    Builder.defineMacro("__RISCV_RVV_VLEN_BITS",
+                        Twine(VScale->first * llvm::RISCV::RVVBitsPerBlock));
 }
----------------
craig.topper wrote:
> 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.
Okay.  So in principle this could be extended to something like a rule where we statically check only that the value is within the specified range, and then it would be dynamically UB to use a type that's wrong for the actual runtime processor?  Maybe that was the idea with SVE but it just never got implemented, which is why the documentation looks the way it does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145088