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