You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by "jerryzj (via GitHub)" <gi...@apache.org> on 2025/04/17 15:57:32 UTC
[PR] RISCV: correct the default VLEN to 128 [tvm]
jerryzj opened a new pull request, #17853:
URL: https://github.com/apache/tvm/pull/17853
- From the RISCV ISA spec
> The V vector extension depends upon the Zvl128b and Zve64d extensions.
Reference: https://github.com/riscv/riscv-isa-manual/blob/e5078e55ea977ef197066dcd3f7c8e09032cb348/src/v-st-ext.adoc?plain=1#L5094
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [RISCV] correct the default VLEN to 128 [tvm]
Posted by "cbalint13 (via GitHub)" <gi...@apache.org>.
cbalint13 commented on PR #17853:
URL: https://github.com/apache/tvm/pull/17853#issuecomment-2816773362
@jerryzj
To further clarify where this ```vector_width``` ends up:
* Targets have different maintainer, having codegen overrides, the original ```vector_width``` is not even used:
https://github.com/search?q=repo%3Aapache%2Ftvm%20native_vector_bits_&type=code
* The only target that really picks it up is the llvm codegen backend:
https://github.com/apache/tvm/blob/36f2502f00d58128451d64c8ff12fa871c994c92/src/target/llvm/codegen_llvm.cc#L176
* In this context, the only place it really propagates is into [GetAlignment()](https://github.com/search?q=repo%3Aapache%2Ftvm+p_native_bits&type=code) , but even here is used as a last resort hint.
Up to this point we can say```vector_width != VLEN``` (no such relationship), but also can't set it to 0 (zero).
E.g. in case of x86 it is set to maximal 512 (for avx512) but in reality only few server class CPU have such vector feature.
Now, in the context of SVE (WiP) it will be really picked up and used to construct a list of ````vscale_range``` hints for arith provers in the SVE predication process. But SVE, in case of riscv, always implies that target have ```v``` extension, and also a minimum default zvl128b flag that will be picked up for sure from LLVM side. In this case ```vector_width == VLEN``` will be true here. This schema also will work for aarch64 SVE predication, the initial hint value of 128 will be used unaltered, this arch doesnt have so fancy flags with direct indicatation on the kind of vector machine.
In last instance if someone really want to enforce other initial hint value, (assuming really knowing what is doing) can override even this initial hint using ```-vector-width=xxx``` but this will get override in the case of exact inference from LLVM side (in case of riscv rvv for sure).
---
Let's set it to 128 as you hinted as a initial hint value, but this PR here failed to pass testcases otherwise it LGTM.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [RISCV] correct the default VLEN to 128 [tvm]
Posted by "cbalint13 (via GitHub)" <gi...@apache.org>.
cbalint13 closed pull request #17853: [RISCV] correct the default VLEN to 128
URL: https://github.com/apache/tvm/pull/17853
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [RISCV] correct the default VLEN to 128 [tvm]
Posted by "jerryzj (via GitHub)" <gi...@apache.org>.
jerryzj commented on PR #17853:
URL: https://github.com/apache/tvm/pull/17853#issuecomment-2816505655
> > Please refer to the ISA spec, the minimum vector length can be set via zvl<?>b
>
> * TVM [is/was](https://github.com/apache/tvm/blob/main/src/target/llvm/llvm_instance.cc?rgh-link-date=2025-04-18T21%3A35%3A29Z#L311-L320) aware of this already.
>
> Your code here touch the case for NO "v" extension presence, where TVM still needs a hint, but lets settle even this to 128.
If there is no `v` or other `zvl` specified, why not set VLEN to 0?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [RISCV] correct the default VLEN to 128 [tvm]
Posted by "cbalint13 (via GitHub)" <gi...@apache.org>.
cbalint13 commented on PR #17853:
URL: https://github.com/apache/tvm/pull/17853#issuecomment-2816682189
> If there is no `v`, `zve*` or other `zvl` specified, meaning that current CPU is not supporting vector operations hence no vector registers, why not set VLEN to 0?
First ```vector_with != VLEN```, it **only becomes** equivalent with VLEN in case of SVE support (WiP), where it will be used.
There is a refactor effort of what is described here: https://github.com/apache/tvm/pull/17859 , making things clear.
---
To recap, TVM is relaying on LLVM (well maintained) as root of trust , it extract "arch knowledge" from, **not vice-versa**:
Few scenarios for ```riscv``` case:
1. If no ```v``` present, or whatever other arch it is, we still need a ```vector_with``` for TVM alignment hints (but doesn't really matter).
2. If only ```v``` is present, but no extra attributes, then a ```zvl128b``` will be implicitly present, it will be added by LLVM library (due to ISA spec you mentioned)
3. If ```v``` is present, and extra attributes like ```+zvl1024b``` as override, then this will be picked up (from LLVM library).
4. If no attributes are present but cpu is a known vendor one like ```-mcpu=sifive-x280``` then ```zvl512b``` will be picked up from LLVM library side.
TVM only extracts ```zvl<XXX>b``` from LLVM library (riscv case), if they do something wrong TVM is also doing wrong.
See full scenarios that are checked as target tests: [tests/python/target/test_riscv_features.py](https://github.com/apache/tvm/blob/4bbdd8c12dd8dfbfbaa1fd86081dd2ca5992eaa8/tests/python/target/test_riscv_features.py)
One can use the sample code (posted here on top) to check what kind of features for various targets are yielded.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [RISCV] correct the default VLEN to 128 [tvm]
Posted by "cbalint13 (via GitHub)" <gi...@apache.org>.
cbalint13 commented on PR #17853:
URL: https://github.com/apache/tvm/pull/17853#issuecomment-2816092581
Hi @jerryzj !
LLVM side (implicitly TVM) is aware of this already:
```
$ cat tvm-llvm-rvv.py
#!/usr/bin/python3
import tvm
target = tvm.target.Target("llvm -device=riscv_cpu -mtriple=riscv32-linux-gnu -mcpu=generic-rv32 -mattr=+i,+m,+v")
print("LLVM [%s]" % target)
print("LLVM features: %s" % (tvm.target.codegen.llvm_get_cpu_features(target)))
```
```
$ ./tvm-llvm-rvv.py
LLVM [llvm -keys=riscv_cpu,cpu -device=riscv_cpu -mattr=+i,+m,+v -mcpu=generic-rv32 -mtriple=riscv32-linux-gnu]
LLVM features: {'zve64x', 'zve32f', 'zve64f', 'zmmul', 'zve64d', 'f', 'zicsr', 'd', 'optimized-nf2-segment-load-store', 'm', 'zve32x', 'zvl32b', 'v', 'zvl128b', 'zvl64b', '32bit', 'i'}
```
Notice the presence of ```zvl128b```, that was added due to ```+v``` presence, so this will be properly picked up anyway.
---
**BUT**, please note that in the case of no ```+v``` (absence) we still need a *default* TVM vector width, and this one has nothing to do with the ISA anymore, but yes let's settle it to 128 (down from current 256).
I am just working right now on refactoring RISCV target to also enable the SVE predication, will reference this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [RISCV] correct the default VLEN to 128 [tvm]
Posted by "cbalint13 (via GitHub)" <gi...@apache.org>.
cbalint13 commented on PR #17853:
URL: https://github.com/apache/tvm/pull/17853#issuecomment-2875728063
Hi @jerryz ,
I merged this along with updated testcases.
This highlight (128 bit defaultness) helped to re-think and simplify down the VLEN parser, by looking solely at zvl<> features, this having guarantees that they are always present and properly set when reading back from LLVM the target's features.
I close this, work here being already in.
Thank you again !
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [RISCV] correct the default VLEN to 128 [tvm]
Posted by "jerryzj (via GitHub)" <gi...@apache.org>.
jerryzj commented on PR #17853:
URL: https://github.com/apache/tvm/pull/17853#issuecomment-2816105844
Please refer to the ISA spec, the minimum vector length can be set via zvl<n>b
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [RISCV] correct the default VLEN to 128 [tvm]
Posted by "cbalint13 (via GitHub)" <gi...@apache.org>.
cbalint13 commented on PR #17853:
URL: https://github.com/apache/tvm/pull/17853#issuecomment-2816230003
> Please refer to the ISA spec, the minimum vector length can be set via zvl<?>b
* TVM [is/was](https://github.com/apache/tvm/blob/main/src/target/llvm/llvm_instance.cc#L311-L320) aware of this already.
Your code here touch the case for NO "v" extension presence, where TVM still needs a hint, but lets settle even this to 128.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org