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

Let MSVC force inline ZSTD_hashPtr() function #2317

Merged
merged 2 commits into from Sep 30, 2020
Merged

Let MSVC force inline ZSTD_hashPtr() function #2317

merged 2 commits into from Sep 30, 2020

Conversation

ghost
Copy link

@ghost ghost commented Sep 21, 2020

ZSTD_hashPtr() function was not expanded by MSVC, led to low performance compared to GCC.

Issue #2314

ZSTD_hashPtr() function was not expanded by MSVC, led to low performance compared to GCC.
@ghost
Copy link
Author

ghost commented Sep 21, 2020

A test failed, seems to be due to this PR:

317compress/zstd_compress_internal.h:650:30: error: ‘ZSTD_hashPtr’ defined but not used [-Werror=unused-function]
318 FORCE_INLINE_TEMPLATE size_t ZSTD_hashPtr(const void* p, U32 hBits, U32 mls)
319                              ^
320cc1: all warnings being treated as errors
321In file included from compress/zstd_compress_superblock.c:18:0:
322compress/zstd_compress_internal.h:650:30: error: ‘ZSTD_hashPtr’ defined but not used [-Werror=unused-function]
323 FORCE_INLINE_TEMPLATE size_t ZSTD_hashPtr(const void* p, U32 hBits, U32 mls)
324                              ^
325cc1: all warnings being treated as errors
326In file included from compress/zstd_ldm.h:18:0,
327                 from compress/zstd_ldm.c:11:
328compress/zstd_compress_internal.h:650:30: error: ‘ZSTD_hashPtr’ defined but not used [-Werror=unused-function]
329 FORCE_INLINE_TEMPLATE size_t ZSTD_hashPtr(const void* p, U32 hBits, U32 mls)
330                              ^
331cc1: all warnings being treated as errors
332In file included from compress/zstdmt_compress.c:27:0:
333compress/zstd_compress_internal.h:650:30: error: ‘ZSTD_hashPtr’ defined but not used [-Werror=unused-function]
334 FORCE_INLINE_TEMPLATE size_t ZSTD_hashPtr(const void* p, U32 hBits, U32 mls)
335                              ^
336cc1: all warnings being treated as errors
337In file included from dictBuilder/fastcover.c:24:0:
338dictBuilder/../compress/zstd_compress_internal.h:650:30: error: ‘ZSTD_hashPtr’ defined but not used [-Werror=unused-function]
339 FORCE_INLINE_TEMPLATE size_t ZSTD_hashPtr(const void* p, U32 hBits, U32 mls)
340                              ^
341cc1: all warnings being treated as errors
342In file included from dictBuilder/zdict.c:51:0:
343dictBuilder/../compress/zstd_compress_internal.h:650:30: error: ‘ZSTD_hashPtr’ defined but not used [-Werror=unused-function]
344 FORCE_INLINE_TEMPLATE size_t ZSTD_hashPtr(const void* p, U32 hBits, U32 mls)
345                              ^
346cc1: all warnings being treated as errors
347Makefile:210: recipe for target 'libzstd.so.1.4.6' failed
348make[1]: *** [libzstd.so.1.4.6] Error 1
349make[1]: Leaving directory '/home/travis/build/facebook/zstd/lib'
350Makefile:63: recipe for target 'lib' failed
351make: *** [lib] Error 2

It adds `__attribute__((unused))` for __GNUC__, to eliminate `-Werror=unused-function` error.
@ghost
Copy link
Author

ghost commented Sep 21, 2020

Change FORCE_INLINE_TEMPLATE to MEM_STATIC FORCE_INLINE_ATTR

Some functions are using this definition:

MEM_STATIC FORCE_INLINE_ATTR
void ZSTD_wildcopy(void* dst, const void* src, ptrdiff_t length, ZSTD_overlap_e const ovtype)

MEM_STATIC FORCE_INLINE_ATTR size_t BIT_getUpperBits(size_t bitContainer, U32 const start)

MEM_STATIC FORCE_INLINE_ATTR size_t BIT_getMiddleBits(size_t bitContainer, U32 const start, U32 const nbBits)

This definition adds an __attribute__((unused)) for __GNUC__.
I tested, it speeded up for MSVC build.

MEM_STATIC:

#if defined(__GNUC__)
#  define MEM_STATIC static __inline __attribute__((unused))
#elif defined (__cplusplus) || (defined (__STDC_VERSION__) && (__STDC_VERSION__ >= 199901L) /* C99 */)
#  define MEM_STATIC static inline
#elif defined(_MSC_VER)
#  define MEM_STATIC static __inline
#else
#  define MEM_STATIC static  /* this version may generate warnings for unused static functions; disable the relevant warning */
#endif

@Cyan4973 Cyan4973 merged commit cc88eb7 into facebook:dev Sep 30, 2020
@ghost ghost deleted the msvc_inline branch October 5, 2020 11:25
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