-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[TEST] CPU feature detection for x86 and ARM dot product instructions #12980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4299b08 to
8e93a57
Compare
8e93a57 to
52bdb86
Compare
python/tvm/testing/utils.py
Outdated
| arch = platform.machine() | ||
| # Only linux is supported for now. | ||
| if arch == "x86_64" and sys.platform.startswith("linux"): | ||
| return "avx512_vnni" in open("/proc/cpuinfo", "r").read() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a bunch of different extensions to vnni. Do we use any of those extensions? If so, we might need another check for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the output of cpuinfo, avx512_vnni is the only relevant extension. There are 8 bit and 16 bit input variants, and we only use the former.
python/tvm/testing/utils.py
Outdated
| requires_arm_dot = Feature("arm_dot", "ARM dot product", run_time_check=_arm_dot_supported) | ||
|
|
||
|
|
||
| requires_cascadelake = Feature("cascadelake", "x86 CascadeLake", run_time_check=_has_vnni) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zen4 supports vnni also. Maybe we need another check to make sure the platform is intel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting... I'll just rename this to requires_vnni, then. There shouldn't be any difference in Intel vs AMD vnni.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests use a cascade lake target though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now, yes. I feel that having both requires_cascadelake and requires_zen4 is redundant. Enabling vnni for zen4 would require minor change in topi x86. So when we make such change, we can parameterize the target string in the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, even if we parameterize the target string to have both -mcpu=cascadelake and -mcpu=zen4, we can't run both targets on the same environment... I should stick to requires_cascadelake, then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a check for intel based on the output from cpuinfo. Only linux is supported for now.
driazati
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know if AWS has machines that support vnni or sdot/udot? We can add them to CI pretty easily if so
Co-authored-by: driazati <9407960+driazati@users.noreply.github.com>
Co-authored-by: driazati <9407960+driazati@users.noreply.github.com>
This reverts commit 6931ca2.
I know that our aarch64 instance supports |
| if line.startswith("hw.optional.arm.FEAT_DotProd"): | ||
| return bool(int(line.split(":", 1)[1])) | ||
| elif sys.platform.startswith("linux"): | ||
| return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a linux aarch64 at hand, so I cannot add the right check here. This is ok for our aarch64 CI since it supports sdot/udot, but it is broken for rasp etc.
I tried return "dot" in open("/proc/cpuinfo", "r").read() but the test was skipped in CI.
…apache#12980) * introduce requires_arm_dot * introduce requires_cascadelake * lint * requires_cascadelake -> requires_vnni * Update tests/python/integration/test_meta_schedule_auto_tensorize.py Co-authored-by: driazati <9407960+driazati@users.noreply.github.com> * Update python/tvm/testing/utils.py Co-authored-by: driazati <9407960+driazati@users.noreply.github.com> * Revert "requires_cascadelake -> requires_vnni" This reverts commit 6931ca2. * check for intel in requires_cascadelake * black * fix Co-authored-by: driazati <9407960+driazati@users.noreply.github.com>
This is a followup to #12865. We have some tests that depend on a dot product instruction of the target architecture, such as VNNI (cascadelake) and ARM
sdot/udot. These tests are currently skipped on CI, but to properly test them on an instance that does support such instructions, I'm introducingrequires_arm_dotandrequires_cascadelakedecorator.@tkonolige @driazati