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

Disable _tzcnt_u64 for ARM64EC #1054

Merged
merged 2 commits into from Jan 27, 2022
Merged

Disable _tzcnt_u64 for ARM64EC #1054

merged 2 commits into from Jan 27, 2022

Conversation

mcfi
Copy link
Contributor

@mcfi mcfi commented Jan 27, 2022

The ARM64EC is a new Microsoft-designed ARM64 ABI that is compatible with AMD64 code. However, not all AMD64 intrinsic functions are supported. For, intrinsics that are lowered to AVX, AVX2 and AVX512 instructions are not supported, including the _tzcnt_u64. To make sure this file compiles for ARM64EC, the use of _tzcnt_u64 should be neutered.

The ARM64EC is a new Microsoft-designed ARM64 ABI that is compatible with AMD64 code. However, not all AMD64 intrinsic functions are supported. For, intrinsics that are lowered to AVX, AVX2 and AVX512 instructions are not supported, including the _tzcnt_u64. To make sure this file compiles for ARM64EC, the use of _tzcnt_u64 should be neutered.
@Cyan4973
Copy link
Member

Interesting,
would there be anyway to test this architecture in CI ?
qemu tests for example ?

@mcfi
Copy link
Contributor Author

mcfi commented Jan 27, 2022

Thanks. I don't know how the current qemu tests work, but if you have an environment with Visual Studio 2022 17.1 preview installed with all the ARM64EC individual components, you may be able to build the lib + all the tests. The tests need to be run on ARM64 windows 11 though.

@Cyan4973
Copy link
Member

Thanks. I don't know how the current qemu tests work, but if you have an environment with Visual Studio 2022 17.1 preview installed with all the ARM64EC individual components, you may be able to build the lib + all the tests. The tests need to be run on ARM64 windows 11 though.

That's unlikely to be compatible with CI unfortunately.

@mcfi
Copy link
Contributor Author

mcfi commented Jan 27, 2022

That's unfortunate, but totally understandable, since the ARM64EC is still in its infancy stage right now. We are building opensource projects as ARM64EC and porting the code if needed. Similar changes have been committed to other projects. See here for another example. :)

@@ -517,7 +517,7 @@ static unsigned LZ4_NbCommonBytes (reg_t val)
assert(val != 0);
if (LZ4_isLittleEndian()) {
if (sizeof(val) == 8) {
# if defined(_MSC_VER) && (_MSC_VER >= 1800) && defined(_M_AMD64) && !defined(LZ4_FORCE_SW_BITCOUNT)
# if defined(_MSC_VER) && (_MSC_VER >= 1800) && (defined(_M_AMD64) && !defined(_M_ARM64EC)) && !defined(LZ4_FORCE_SW_BITCOUNT)
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a one-line comment explaining why _M_ARM64EC must be exclude ?
Essentially a summary of what you already explain in the PR, just hosted directly in the code source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I just added comments.

Copy link
Member

@Cyan4973 Cyan4973 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @mcfi !

@mcfi
Copy link
Contributor Author

mcfi commented Jan 27, 2022

Thank you for your review!

@Cyan4973 Cyan4973 merged commit 5cc3118 into lz4:dev Jan 27, 2022
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.

None yet

2 participants