Skip to content

Disable _tzcnt_u64 for ARM64EC#1054

Merged
Cyan4973 merged 2 commits intolz4:devfrom
mcfi:patch-1
Jan 27, 2022
Merged

Disable _tzcnt_u64 for ARM64EC#1054
Cyan4973 merged 2 commits intolz4:devfrom
mcfi:patch-1

Conversation

@mcfi
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@mcfi
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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. :)

Comment thread lib/lz4.c
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

2 participants