[PATCH] D107290: [PoC][RISCV] Add support for the vscale_range attribute

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: clang/lib/Basic/Targets/RISCV.cpp:349
+  unsigned VLENMax = 65536;
+  return std::make_pair(VLENMin / 64, VLENMax / 64);
+}
----------------
craig.topper wrote:
> Should we move RVVBitsPerBlock to RISCVTargetParser.def? Or some other place that can be shared between lllvm/lib/Target/RISCV/ and here?
Good idea. I also added the "StdV" min/max values of `128`/`65536` in there. However, I just put them in `TargetParser.h` as putting them in the `.def`  file felt a bit odd and you had to account for preprocessor logic. It still feels a little odd but I agree that sharing these values is important. Other targets have specific values in there so it's not unprecedented. It is target-adjacent data, even if it's not (currently) dependent on triples or cpus.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:106
+  assert(RVVBitsMin % 128 == 0 &&
+         "RVV requires vector length in multiples of 128!");
+  assert(RVVBitsMax % 128 == 0 &&
----------------
kito-cheng wrote:
> RISC-V require VLEN in power of 2, multiples of 128 is constraint for SVE :p
> https://github.com/riscv/riscv-v-spec/blob/master/v-spec.adoc#2-implementation-defined-constant-parameters
Yeah to be honest I was just being cheeky/lazy here :) Since our current implementation requires `VLEN >= 128` we know that VLEN must always be a multiple of 128. But yes this isn't really the right way of coding it, even if it does the right thing. I've fixed that up now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107290