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

Check STATIC_BMI2 instead of __BMI2__ #4256

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Conversation

pps83
Copy link
Contributor

@pps83 pps83 commented Jan 20, 2025

ZSTD_ENABLE_ASM_X86_64_BMI2 already implies BMI2 by its name, and by the way it tests defines

@pps83 pps83 changed the title Remove redundant BMI2 check when Remove redundant BMI2 check Jan 21, 2025
@Cyan4973 Cyan4973 self-assigned this Jan 23, 2025
@Cyan4973
Copy link
Contributor

Looking at the definition of ZSTD_ENABLE_ASM_X86_64_BMI2,
it appears it does not imply the presence of __BMI2__.

The full test is :

#if !defined(ZSTD_DISABLE_ASM) &&                       \
    ZSTD_ASM_SUPPORTED &&                               \
    defined(__x86_64__) &&                              \
    (DYNAMIC_BMI2 || defined(__BMI2__))

So, ZSTD_ENABLE_ASM_X86_64_BMI2 can be defined to 1 if DYNAMIC_BMI2 is set to 1 too,
even if __BMI2__ is not defined.

It follows that ZSTD_ENABLE_ASM_X86_64_BMI2 && defined(__BMI2__) is not the same test as ZSTD_ENABLE_ASM_X86_64_BMI2 alone.
So the proposed change is not obvious to validate.

The context into which this macro test is invoked is also a bit complex, or at least it's not trivial to me.
I just note that, before this block, there are other blocks also triggered by similar though different macro tests,
such as #if DYNAMIC_BMI2, and # if ZSTD_ENABLE_ASM_X86_64_BMI2,
and they do something similar, such as setting loopFn function pointer.

I would look for @terrelln guidance on this code block,
and in absence of such guidance, I would rather lean towards cautiousness and not validate the suggested modification.

@pps83
Copy link
Contributor Author

pps83 commented Jan 23, 2025

Yep, you are right, it's not exact.

Please, ignore this one for now. Imo it should be refactored with STATIC_BMI instead

@Cyan4973 Cyan4973 marked this pull request as draft January 23, 2025 22:08
pps83 added 2 commits January 30, 2025 22:16
`ZSTD_ENABLE_ASM_X86_64_BMI2` already implies BMI2 by its name, and by the way it's defined
@pps83 pps83 changed the title Remove redundant BMI2 check Check STATIC_BMI2 instead of __BMI2__ Jan 31, 2025
@pps83 pps83 marked this pull request as ready for review January 31, 2025 17:59
@pps83
Copy link
Contributor Author

pps83 commented Jan 31, 2025

Please, ignore this one for now. Imo it should be refactored with STATIC_BMI instead

should be ok now.

@pps83
Copy link
Contributor Author

pps83 commented Jan 31, 2025

linux-kernel fails with this error:

../linux/lib/zstd/compress/../common/portability_macros.h:66:11: error: "STATIC_BMI2" is not defined, evaluates to 0 [-Werror=undef]
   66 |       && !STATIC_BMI2

however, there is no STATIC_BMI2 on portability_macros.h:66 line

@Cyan4973
Copy link
Contributor

Try to rebase your PR on top of latest dev branch commit

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants