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

Guard uses of __builtin_clz and __builtin_clzll #884

Closed
wants to merge 1 commit into from

Conversation

allanjude
Copy link
Contributor

Prevents undefined reference to `__clzdi2' on FreeBSD sparc64 compiled with gcc 4.2

Fallback to the software implementation by making the preprocessor guard better

Prevents undefined reference to `__clzdi2' on FreeBSD sparc64 compiled with gcc 4.2

Fallback to the software implementation by making the preprocessor guard better
@allanjude
Copy link
Contributor Author

I see that this only works in clang, and does not fail in gcc on FreeBSD because of a compatibility macro.

Will need to revisit this

@Cyan4973
Copy link
Contributor

Cyan4973 commented Oct 9, 2017

Maybe using __GNUC_MINOR__ ?

Also, is there a difference to do between __builtin_clz and __builtin_clzll, the 32-bits version being more "commonly" supported ?

@Cyan4973 Cyan4973 closed this Nov 27, 2017
@cemeyer
Copy link
Contributor

cemeyer commented Mar 14, 2018

We could provide a similar compatibility macro in Zstd, right Allan?

As it is, the lack of (something like) this revision in Zstandard makes backporting new versions to FreeBSD more painful than necessary. (Some of our architectures do not provide __builtin_clz or __builtin_clzll.)

Or even something dumber like a configure-style HAVE_BUILTIN_CLZ / HAVE_BUILTIN_CLZLL we can enable/disable based on platform defines in one location?

@Cyan4973
Copy link
Contributor

note that __has_builtin is clang specific, it will not work on gcc.

@allanjude
Copy link
Contributor Author

Yeah, the problem with the FreeBSD compatibility macro, is that is just returns false, which would be a large pessimization on modern GCC that does have it.

@cemeyer
Copy link
Contributor

cemeyer commented Mar 14, 2018

Sure. We could do HAVE_BUILTIN_CLZ(LL) instead of the specific checks for GCC versions. There is precedent in the tree, e.g., DYNAMIC_BMI2.

@terrelln
Copy link
Contributor

Yeah, we can definitely make a macro for the test in compiler.h so updates only have to change one place.

uqs pushed a commit to freebsd/freebsd-src that referenced this pull request Mar 14, 2018
Includes patch to conditionalize use of __builtin_clz(ll) on __has_builtin().
The issue is tracked upstream at facebook/zstd#884 .
Otherwise, these are vanilla Zstandard 1.3.3 files.

Note that the 1.3.4 release should be due out soon.

Sponsored by:	Dell EMC Isilon


git-svn-id: svn+ssh://svn.freebsd.org/base/head@330894 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f
uqs pushed a commit to freebsd/freebsd-src that referenced this pull request Mar 14, 2018
Includes patch to conditionalize use of __builtin_clz(ll) on __has_builtin().
The issue is tracked upstream at facebook/zstd#884 .
Otherwise, these are vanilla Zstandard 1.3.3 files.

Note that the 1.3.4 release should be due out soon.

Sponsored by:	Dell EMC Isilon
@tsoome
Copy link

tsoome commented Mar 23, 2018

There is also an problem that even in (recent) gcc, the __has_builtin() itself may be missing; meaning that lib/common/compiler.h should have something like:

#ifdef __has_builtin
#define HAS_BUILTIN(x) __has_builtin(x)
#else
#define HAS_BUILTIN(x) 0
#endif

and then you can use HAS_BUILTIN() ...

@cemeyer
Copy link
Contributor

cemeyer commented Mar 23, 2018

@tsoome Yeah, that is basically the compatibility macro that just returns false that @allanjude is describing.

Instead we are suggesting specific HAVE_BUILTIN_FOO macros for the feature in compiler.h, that can be override by distro/architectures in one place. Then code that uses __builtin_clz or whatever is guarded by one of those macros.

mat813 pushed a commit to mat813/freebsd that referenced this pull request Mar 26, 2018
Includes patch to conditionalize use of __builtin_clz(ll) on __has_builtin().
The issue is tracked upstream at facebook/zstd#884 .
Otherwise, these are vanilla Zstandard 1.3.3 files.

Note that the 1.3.4 release should be due out soon.

Sponsored by:	Dell EMC Isilon


git-svn-id: https://svn.freebsd.org/base/head@330894 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f
uqs pushed a commit to freebsd/freebsd-src that referenced this pull request Mar 27, 2018
Includes our local patch to conditionalize use of __builtin_clz(ll) on
Clang's __has_builtin() (which is just defined to false when building with
GCC).

The issue is tracked upstream at facebook/zstd#884 .
Otherwise, these are vanilla Zstandard 1.3.4 files.

Reported by:	allanjude, Yann Collet
Sponsored by:	Dell EMC Isilon


git-svn-id: svn+ssh://svn.freebsd.org/base/head@331602 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f
uqs pushed a commit to freebsd/freebsd-src that referenced this pull request Mar 27, 2018
Includes our local patch to conditionalize use of __builtin_clz(ll) on
Clang's __has_builtin() (which is just defined to false when building with
GCC).

The issue is tracked upstream at facebook/zstd#884 .
Otherwise, these are vanilla Zstandard 1.3.4 files.

Reported by:	allanjude, Yann Collet
Sponsored by:	Dell EMC Isilon
bdrewery pushed a commit to bdrewery/freebsd that referenced this pull request Apr 2, 2018
Includes patch to conditionalize use of __builtin_clz(ll) on __has_builtin().
The issue is tracked upstream at facebook/zstd#884 .
Otherwise, these are vanilla Zstandard 1.3.3 files.

Note that the 1.3.4 release should be due out soon.

Sponsored by:	Dell EMC Isilon


git-svn-id: svn+ssh://svn.freebsd.org/base/head@330894 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f
bdrewery pushed a commit to bdrewery/freebsd that referenced this pull request Apr 2, 2018
Includes our local patch to conditionalize use of __builtin_clz(ll) on
Clang's __has_builtin() (which is just defined to false when building with
GCC).

The issue is tracked upstream at facebook/zstd#884 .
Otherwise, these are vanilla Zstandard 1.3.4 files.

Reported by:	allanjude, Yann Collet
Sponsored by:	Dell EMC Isilon


git-svn-id: svn+ssh://svn.freebsd.org/base/head@331602 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f
mat813 pushed a commit to mat813/freebsd that referenced this pull request Apr 9, 2018
Includes our local patch to conditionalize use of __builtin_clz(ll) on
Clang's __has_builtin() (which is just defined to false when building with
GCC).

The issue is tracked upstream at facebook/zstd#884 .
Otherwise, these are vanilla Zstandard 1.3.4 files.

Reported by:	allanjude, Yann Collet
Sponsored by:	Dell EMC Isilon


git-svn-id: https://svn.freebsd.org/base/head@331602 ccf9f872-aa2e-dd11-9fc8-001c23d0bc1f
brooksdavis pushed a commit to CTSRD-CHERI/cheribsd that referenced this pull request Aug 22, 2018
Includes patch to conditionalize use of __builtin_clz(ll) on __has_builtin().
The issue is tracked upstream at facebook/zstd#884 .
Otherwise, these are vanilla Zstandard 1.3.3 files.

Note that the 1.3.4 release should be due out soon.

Sponsored by:	Dell EMC Isilon
brooksdavis pushed a commit to CTSRD-CHERI/cheribsd that referenced this pull request Aug 23, 2018
Includes our local patch to conditionalize use of __builtin_clz(ll) on
Clang's __has_builtin() (which is just defined to false when building with
GCC).

The issue is tracked upstream at facebook/zstd#884 .
Otherwise, these are vanilla Zstandard 1.3.4 files.

Reported by:	allanjude, Yann Collet
Sponsored by:	Dell EMC Isilon
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.

6 participants