cbalint13 commented on PR #16425:
URL: https://github.com/apache/tvm/pull/16425#issuecomment-1898307643
Hi @lhutton1 !
Thanks a lot for picking up the recently introduce llvm reflection (well,
kind of) for ARM targets !
I also believe it's a far better to query llvm submodule own inventory than
"guessing" what llvm might want.
> Currently, target features are determined by a set of fixed checks on the
target string. This works well for checking support of a small number of simple
features, but it doesn't scale. Some problems include:
>
> * There are many non-trivial conditions for which a feature may(not) be
available. It is easy to miss these with the current implementation.
> * The inclusion of some features in a target string can imply other
features. For example, "+sve2" implies "+sve". This currently isn't taken into
account.
> * The tests in tests/cpp/target/parsers/aprofile_test.c suggest that
targets such as "llvm -mcpu=cortex-a+neon" and "llvm -mattr=+noneon" are
supported target strings. The features will be correctly parsed in TVM,
however, they are not valid in LLVM. Therefore, it's possible that TVM and LLVM
have different understanding of the features available.
Yes I agree, I was also concerned about, all the mentioned target strings
are "non-legit" from llvm point of view.
To reiterate, if one want to use llvm targets properly than the flags
concerning llvm submodule should:
* Target **must have** a ```-mtriple=<triple>``` and the ```triple```
should be **only** a arch triple.
* Target **may have** a ```-mcpu=<cpumodel>``` and the ```cpumodel```
should be **valid** with the ```mtriple``` arch.
* Target **may have** a ```-mattr=<cpuflags>```, comma separated list
where ```'+feat'``` add, ```'-feat'``` subtracts features.
> This commit uses the more robust LLVM target parser to determine support
for the features in TVM. It leverages previous infrastructure added to TVM for
obtaining a list of all supported features given an input target, and uses this
to check the existance of certain features we're interested in. It should be
trivial to grow this list over time. As a result of this change, the problems
mentioned above are solved.
>
> In the current form, this commit drops support for target strings such as
"llvm -mcpu=cortex-a+neon" and "llvm -mattr=+noneon". A scan of the codebase
suggests this functionality is not in use (only in test cases). Should we feel
the need to support them, or have a smoother migration for downstream users of
TVM we can add a translator to the parser to convert these into LLVM compatible
targets.
As you also underlined subtracting a feature may disable whole set of
futures, and llvm do it being cpu+arch aware.
Addition or subtraction can be done with ```+feat``` or ```-feat```,
a.f.a.i.k. there is no such thing as ```+no<whatever>``` in llvm.
Here is an example subtracting feature (avx512f aka foundation) that leads
to complete disablement of **all** avx512:
```
target = tvm.target.Target("llvm -mcpu=sapphirerapids")
print("BEFORE", tvm.target.codegen.llvm_get_cpu_features(target) )
print()
target = tvm.target.Target("llvm -mcpu=sapphirerapids -mattr=-avx512f")
print( "AFTER", tvm.target.codegen.llvm_get_cpu_features(target) )
["64bit", "64bit-mode", "adx", "aes", "allow-light-256-bit", "amx-bf16",
"amx-int8", "amx-tile", "avx", "avx2", "avx512bf16", "avx512bitalg",
"avx512bw", "avx512cd", "avx512dq", "avx512f", "avx512fp16",
"avx512ifma", "avx512vbmi", "avx512vbmi2", "avx512vl", "avx512vnni",
"avx512vpopcntdq", "avxvnni", ....]
["64bit", "64bit-mode", "adx", "aes", "allow-light-256-bit", "amx-bf16",
"amx-int8", "amx-tile", "avx", "avx2", "avxvnni", ... ]
```
I salute this PR, thanks again @lhutton1 !
--
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-unsubscr...@tvm.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
