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

Move STATIC_BMI2 define to portability_macros.h #4264

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

Conversation

pps83
Copy link
Contributor

@pps83 pps83 commented Jan 23, 2025

This change makes sure that STATIC_BMI and DYNAMIC_BMI are defined close to each other, and not in other headers.
Also, it reorders __BMI2__ check:

  • if __BMI2__ defined, then set STATIC_BMI2 for all compilers
  • use defined(_MSC_VER) && defined(__AVX2__) as fallback for ms compiler

@pps83
Copy link
Contributor Author

pps83 commented Jan 23, 2025

Note, portability_macros.h is included only in 3 places:

  1. compiler.h
  2. zstd_cwksp.h
  3. huf_decompress_amd64.S

compiler.h does include portability_macros.h.
zstd_cwksp.h includes compiler.h right after portability_macros.h
huf_decompress_amd64.S is asm file, doesn't check for STATIC_BMI

@Cyan4973 Cyan4973 self-assigned this Jan 23, 2025
@Cyan4973
Copy link
Contributor

Cyan4973 commented Jan 23, 2025

I'm supportive of this change.

I'm also wondering if DYNAMIC_BMI2 is impacted by this welcome ordering change.
I would have expected DYNAMIC_BMI2 to depend on the state of STATIC_BMI2.

@Cyan4973
Copy link
Contributor

Also, cc @terrelln , since it might be relevant to kernel code and assembly units

@pps83
Copy link
Contributor Author

pps83 commented Jan 23, 2025

I would have expected DYNAMIC_BMI2 to depend on the state of STATIC_BMI2.

yep, but then I get more and more changes on top :)
in short, my understanding: DYNAMIC_BMI2 should only be set to 1 if STATIC_BMI2 is 0. But then, looking into the code, it seems it would affect it, as the checks do not seem to be consistent. And in some cases, code is quite questionable.

pps83 added 3 commits January 24, 2025 03:02
 + if `__BMI2__` defined, then set STATIC_BMI2 for all compilers
 + use `defined(_MSC_VER) && defined(__AVX2__)` as fallback for ms compiler
@pps83
Copy link
Contributor Author

pps83 commented Jan 27, 2025

@terrelln ping

@pps83
Copy link
Contributor Author

pps83 commented Jan 28, 2025

yep, but then I get more and more changes on top :) in short, my understanding: DYNAMIC_BMI2 should only be set to 1 if STATIC_BMI2 is 0. But then, looking into the code, it seems it would affect it, as the checks do not seem to be consistent. And in some cases, code is quite questionable.

After the change, step by step changes:

  • there are only a few places where __BMI2__ is used, it should be replaced with STATIC_BMI2 (including this one)
  • there is a place with the check if (defined(__BMI__) || defined(__BMI2__)) && defined(__GNUC__). Perhaps, checking for __BMI__ is enough: if __BMI2__ is enabled, that means that __BMI__ is also enabled.

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