Skip to content
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

Fix BF16 detection in x86_64 ukernel cmake #17983

Merged

Conversation

vient
Copy link
Contributor

@vient vient commented Jul 23, 2024

_mm512_cvtneps_pbh was used incorrectly in IREE_UK_BUILD_X86_64_AVX512_BF16 cmake test, so it always failed.

@vient vient requested a review from benvanik as a code owner July 23, 2024 00:25
@vient
Copy link
Contributor Author

vient commented Jul 23, 2024

Don't know if it affects anything. Surely not in runtime? I just tried to build IREE for the first time and noticed Performing Test IREE_UK_BUILD_X86_64_AVX512_BF16 - Failed in cmake output.

https://godbolt.org/z/WzEsYv1hK just in case

@ScottTodd ScottTodd requested a review from bjacob July 23, 2024 00:41
Copy link
Contributor

@bjacob bjacob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I had been wondering why it was failing 🤦 it does not affect real functionality, only tests. Real functionality depends on the bitcode build of ukernels which has these features unconditional.

`_mm512_cvtneps_pbh` was used incorrectly in IREE_UK_BUILD_X86_64_AVX512_BF16 cmake test

Signed-off-by: vient <[email protected]>
@vient vient force-pushed the users/vient/fix-x86_64-bf16-detection branch from 2f99e30 to ffe9123 Compare July 23, 2024 01:31
@vient
Copy link
Contributor Author

vient commented Aug 8, 2024

So, do I need to do anything else here, or this pull request just got lost? "2 expected checks" don't know what to do about these.

@ScottTodd
Copy link
Member

So, do I need to do anything else here, or this pull request just got lost? "2 expected checks" don't know what to do about these.

Thanks for the ping.

Regular contributors usually have write access and can merge their own PRs. We sometimes need a reminder when that isn't the case and we'll merge for you: https://iree.dev/developers/general/contributing/#merging-approved-changes.

As for the expected checks, those workflows were added recently and the commit this was based at predates them. If you sync your PR branch (e.g. by rebasing onto main or merging main into your branch), those checks should run. In this case though, I'm comfortable just bypassing the branch protection / required checks since the tests that ran when this was sent should be sufficient.

@ScottTodd ScottTodd merged commit e57ccaa into iree-org:main Aug 8, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants