You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/06/19 20:27:34 UTC
[GitHub] [incubator-nuttx] nimish opened a new pull request, #6482: Use builtins for byteswapping
nimish opened a new pull request, #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482
Make use of XCHG/BSWAP on x86, REV16 and REV on ARMv6-m and above, and whatever other optimized instructions on other platforms.
## Summary
Use compiler builtins where available
## Impact
A little speed boost
## Testing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#issuecomment-1167728916
> @nimish please squash the second patch into the first.
and the style warning:
```
../nuttx/tools/checkpatch.sh -m -g 4c2b7f3ac9052d50aa1c8fa20e643ce4bbf639c6..HEAD
Error: /home/runner/work/incubator-nuttx/incubator-nuttx/nuttx/include/nuttx/compiler.h:120:1: error: Too many blank lines
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r907041715
##########
include/endian.h:
##########
@@ -51,23 +51,40 @@
/* Common byte swapping macros */
-#define __SWAP_UINT16_ISMACRO 1
-#undef __SWAP_UINT32_ISMACRO
-
-#ifdef __SWAP_UINT16_ISMACRO
+#ifdef CONFIG_HAVE_BUILTIN_BSWAP16
+# define __swap_uint16 __builtin_bswap16
+#else
# define __swap_uint16(n) \
(uint16_t)(((((uint16_t)(n)) & 0x00ff) << 8) | \
((((uint16_t)(n)) >> 8) & 0x00ff))
#endif
-#ifdef __SWAP_UINT32_ISMACRO
+#ifdef CONFIG_HAVE_BUILTIN_BSWAP32
+# define __swap_uint32 __builtin_bswap32
+#else
# define __swap_uint32(n) \
(uint32_t)(((((uint32_t)(n)) & 0x000000ffUL) << 24) | \
((((uint32_t)(n)) & 0x0000ff00UL) << 8) | \
((((uint32_t)(n)) & 0x00ff0000UL) >> 8) | \
((((uint32_t)(n)) & 0xff000000UL) >> 24))
#endif
+#ifdef CONFIG_HAVE_LONG_LONG
+# ifdef CONFIG_HAVE_BUILTIN_BSWAP64
+# define __swap_uint64 __builtin_bswap64
+# else
+# define __swap_uint64(n) \
+ (uint64_t)(((((uint64_t)(n)) & 0x00000000000000ffULL) << 56) | \
+ ((((uint64_t)(n)) & 0x000000000000ff00ULL) << 40) | \
+ ((((uint64_t)(n)) & 0x0000000000ff0000ULL) << 24) | \
+ ((((uint64_t)(n)) & 0x00000000ff000000ULL) << 8) | \
+ ((((uint64_t)(n)) & 0x000000ff00000000ULL) >> 8) | \
+ ((((uint64_t)(n)) & 0x0000ff0000000000ULL) >> 24) | \
+ ((((uint64_t)(n)) & 0x00ff000000000000ULL) >> 40) | \
+ ((((uint64_t)(n)) & 0xff00000000000000ULL) >> 56))
Review Comment:
```suggestion
(uint64_t)(((((uint64_t)(n)) & 0x00000000000000ffULL) << 56) | \
((((uint64_t)(n)) & 0x000000000000ff00ULL) << 40) | \
((((uint64_t)(n)) & 0x0000000000ff0000ULL) << 24) | \
((((uint64_t)(n)) & 0x00000000ff000000ULL) << 8) | \
((((uint64_t)(n)) & 0x000000ff00000000ULL) >> 8) | \
((((uint64_t)(n)) & 0x0000ff0000000000ULL) >> 24) | \
((((uint64_t)(n)) & 0x00ff000000000000ULL) >> 40) | \
((((uint64_t)(n)) & 0xff00000000000000ULL) >> 56))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] nimish commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
nimish commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r903006533
##########
include/netinet/in.h:
##########
@@ -232,7 +232,7 @@
((((unsigned short)(ns)) >> 8) & 0x00ff))
# endif
# ifdef __has_builtin && __has_builtin(__builtin_bswap32)
-# define HTONL(nl) ((unsigned long)__builtin_bswap32(((unsigned short) (nl))))
+# define HTONL(nl) ((unsigned long)__builtin_bswap32(((unsigned long) (nl))))
Review Comment:
squash merge it at the end?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r903382876
##########
include/nuttx/compiler.h:
##########
@@ -69,6 +69,36 @@
# undef CONFIG_HAVE_CXX14
#endif
+#if defined(__cplusplus) && __cplusplus >= 201703L
+# define CONFIG_HAVE_CXX17 1
+#else
+# undef CONFIG_HAVE_CXX17
+#endif
+
+
+/* Check for intrinsics */
+#ifdef __has_builtin
+# if __has_builtin(__builtin_bswap16)
+# define CONFIG_HAVE_BUILTIN_BSWAP16 1
+# endif
+# if __has_builtin(__builtin_bswap32)
+# define CONFIG_HAVE_BUILTIN_BSWAP32 1
+# endif
+# if __has_builtin(__builtin_bswap64) && defined(CONFIG_HAVE_LONG_LONG)
+# define CONFIG_HAVE_BUILTIN_BSWAP64 1
+# endif
+# if __has_builtin(__builtin_ctz)
+# define CONFIG_HAVE_BUILTIN_CTZ 1
+# endif
+# if __has_builtin(__builtin_clz)
+# define CONFIG_HAVE_BUILTIN_CLZ 1
+# endif
+# if __has_builtin(__builtin_popcount)
+# define CONFIG_HAVE_BUILTIN_POPCOUNT 1
+# endif
+#endif
Review Comment:
Please move all this block under `#ifdef __GNUC__`.
Also according https://stackoverflow.com/questions/54079257/how-to-check-builtin-function-is-available-on-gcc the `__has_builtin` is a new GCC feature while the old version might still have builtin functions available, but will fail `__has_builtin` check. It is better to rely on the GCC version.
And please use 2 spaces indentation.
##########
include/netinet/in.h:
##########
@@ -217,26 +217,16 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
*/
#ifdef CONFIG_ENDIAN_BIG
# define HTONS(ns) (ns)
# define HTONL(nl) (nl)
+#elif defined (CONFIG_ENDIAN_LITTLE)
+# define HTONS(ns) ((uint16_t)__swap_uint16(((uint16_t) (ns))))
+# define HTONL(nl) ((uint32_t)__swap_uint32(((uint32_t) (nl))))
#else
-# define HTONS(ns) \
- (unsigned short) \
- (((((unsigned short)(ns)) & 0x00ff) << 8) | \
- ((((unsigned short)(ns)) >> 8) & 0x00ff))
-# define HTONL(nl) \
- (unsigned long) \
- (((((unsigned long)(nl)) & 0x000000ffUL) << 24) | \
- ((((unsigned long)(nl)) & 0x0000ff00UL) << 8) | \
- ((((unsigned long)(nl)) & 0x00ff0000UL) >> 8) | \
- ((((unsigned long)(nl)) & 0xff000000UL) >> 24))
+# error "PDP-11s are not supported."
Review Comment:
```suggestion
# error "PDP-11s are not supported."
```
##########
include/netinet/in.h:
##########
@@ -217,26 +217,16 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
*/
#ifdef CONFIG_ENDIAN_BIG
# define HTONS(ns) (ns)
# define HTONL(nl) (nl)
+#elif defined (CONFIG_ENDIAN_LITTLE)
Review Comment:
```suggestion
#elif defined(CONFIG_ENDIAN_LITTLE)
```
##########
include/netinet/in.h:
##########
@@ -217,26 +217,16 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
*/
#ifdef CONFIG_ENDIAN_BIG
# define HTONS(ns) (ns)
# define HTONL(nl) (nl)
Review Comment:
```suggestion
# define HTONS(ns) (ns)
# define HTONL(nl) (nl)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r904858886
##########
include/endian.h:
##########
@@ -52,21 +52,50 @@
/* Common byte swapping macros */
#define __SWAP_UINT16_ISMACRO 1
-#undef __SWAP_UINT32_ISMACRO
+#define __SWAP_UINT32_ISMACRO 1
+#define __SWAP_UINT64_ISMACRO 1
+
#ifdef __SWAP_UINT16_ISMACRO
-# define __swap_uint16(n) \
+# ifdef CONFIG_HAVE_BUILTIN_BSWAP16
+# define __swap_uint16(n) ((uint16_t)__builtin_bswap16((uint32_t)(n)))
+# else
+# define __swap_uint16(n) \
(uint16_t)(((((uint16_t)(n)) & 0x00ff) << 8) | \
((((uint16_t)(n)) >> 8) & 0x00ff))
+# endif
#endif
#ifdef __SWAP_UINT32_ISMACRO
+#ifdef CONFIG_HAVE_BUILTIN_BSWAP32
+# define __swap_uint32(n) \
+ ((uint32_t)__builtin_bswap32((uint32_t)(n)))
+#else
# define __swap_uint32(n) \
(uint32_t)(((((uint32_t)(n)) & 0x000000ffUL) << 24) | \
((((uint32_t)(n)) & 0x0000ff00UL) << 8) | \
((((uint32_t)(n)) & 0x00ff0000UL) >> 8) | \
((((uint32_t)(n)) & 0xff000000UL) >> 24))
#endif
+#endif
+
+#ifdef __SWAP_UINT64_ISMACRO && CONFIG_HAVE_LONG_LONG
+#ifdef CONFIG_HAVE_BUILTIN_BSWAP64
+# define __swap_uint64(n) \
+ ((uint64_t)__builtin_bswap64((uint64_t)(n)))
Review Comment:
```suggestion
# define __swap_uint64 __builtin_bswap64
```
##########
include/endian.h:
##########
@@ -52,21 +52,50 @@
/* Common byte swapping macros */
#define __SWAP_UINT16_ISMACRO 1
-#undef __SWAP_UINT32_ISMACRO
+#define __SWAP_UINT32_ISMACRO 1
+#define __SWAP_UINT64_ISMACRO 1
+
#ifdef __SWAP_UINT16_ISMACRO
-# define __swap_uint16(n) \
+# ifdef CONFIG_HAVE_BUILTIN_BSWAP16
+# define __swap_uint16(n) ((uint16_t)__builtin_bswap16((uint32_t)(n)))
+# else
+# define __swap_uint16(n) \
(uint16_t)(((((uint16_t)(n)) & 0x00ff) << 8) | \
((((uint16_t)(n)) >> 8) & 0x00ff))
+# endif
#endif
#ifdef __SWAP_UINT32_ISMACRO
+#ifdef CONFIG_HAVE_BUILTIN_BSWAP32
+# define __swap_uint32(n) \
+ ((uint32_t)__builtin_bswap32((uint32_t)(n)))
Review Comment:
```suggestion
# define __swap_uint32 __builtin_bswap32
```
##########
include/nuttx/compiler.h:
##########
@@ -69,6 +69,36 @@
# undef CONFIG_HAVE_CXX14
#endif
+#if defined(__cplusplus) && __cplusplus >= 201703L
+# define CONFIG_HAVE_CXX17 1
+#else
+# undef CONFIG_HAVE_CXX17
+#endif
+
+
+/* Check for intrinsics */
+#ifdef __has_builtin
+# if __has_builtin(__builtin_bswap16)
+# define CONFIG_HAVE_BUILTIN_BSWAP16 1
+# endif
+# if __has_builtin(__builtin_bswap32)
+# define CONFIG_HAVE_BUILTIN_BSWAP32 1
+# endif
+# if __has_builtin(__builtin_bswap64) && defined(CONFIG_HAVE_LONG_LONG)
+# define CONFIG_HAVE_BUILTIN_BSWAP64 1
+# endif
+# if __has_builtin(__builtin_ctz)
+# define CONFIG_HAVE_BUILTIN_CTZ 1
+# endif
+# if __has_builtin(__builtin_clz)
+# define CONFIG_HAVE_BUILTIN_CLZ 1
+# endif
+# if __has_builtin(__builtin_popcount)
+# define CONFIG_HAVE_BUILTIN_POPCOUNT 1
+# endif
+#endif
Review Comment:
I think Clag also defines `__GNUC__`
##########
include/endian.h:
##########
@@ -52,21 +52,50 @@
/* Common byte swapping macros */
#define __SWAP_UINT16_ISMACRO 1
-#undef __SWAP_UINT32_ISMACRO
+#define __SWAP_UINT32_ISMACRO 1
+#define __SWAP_UINT64_ISMACRO 1
+
#ifdef __SWAP_UINT16_ISMACRO
-# define __swap_uint16(n) \
+# ifdef CONFIG_HAVE_BUILTIN_BSWAP16
+# define __swap_uint16(n) ((uint16_t)__builtin_bswap16((uint32_t)(n)))
Review Comment:
```suggestion
# define __swap_uint16 __builtin_bswap16
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] nimish commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
nimish commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r904015237
##########
include/netinet/in.h:
##########
@@ -217,26 +217,16 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
Review Comment:
* Unclear if we need it given we aren't using the original swaps
* It appears to conflict with the top-level copyright header (all rights reserved vs Apache)
No objection to keeping it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] nimish commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
nimish commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r906397556
##########
include/nuttx/compiler.h:
##########
@@ -69,6 +69,36 @@
# undef CONFIG_HAVE_CXX14
#endif
+#if defined(__cplusplus) && __cplusplus >= 201703L
+# define CONFIG_HAVE_CXX17 1
+#else
+# undef CONFIG_HAVE_CXX17
+#endif
+
+
+/* Check for intrinsics */
+#ifdef __has_builtin
+# if __has_builtin(__builtin_bswap16)
+# define CONFIG_HAVE_BUILTIN_BSWAP16 1
+# endif
+# if __has_builtin(__builtin_bswap32)
+# define CONFIG_HAVE_BUILTIN_BSWAP32 1
+# endif
+# if __has_builtin(__builtin_bswap64) && defined(CONFIG_HAVE_LONG_LONG)
+# define CONFIG_HAVE_BUILTIN_BSWAP64 1
+# endif
+# if __has_builtin(__builtin_ctz)
+# define CONFIG_HAVE_BUILTIN_CTZ 1
+# endif
+# if __has_builtin(__builtin_clz)
+# define CONFIG_HAVE_BUILTIN_CLZ 1
+# endif
+# if __has_builtin(__builtin_popcount)
+# define CONFIG_HAVE_BUILTIN_POPCOUNT 1
+# endif
+#endif
Review Comment:
done. Clang does unless you disable it and it may not keep up with GCC's one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#issuecomment-1166806885
@nimish why close this PR? will you create a new one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r901816203
##########
include/netinet/in.h:
##########
@@ -217,26 +217,30 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
*/
#ifdef CONFIG_ENDIAN_BIG
# define HTONS(ns) (ns)
# define HTONL(nl) (nl)
#else
+# ifdef __has_builtin && __has_builtin(__builtin_bswap16)
+# define HTONS(ns) ((unsigned short)__builtin_bswap16(((unsigned short) (ns))))
Review Comment:
Maybe adding something similar `CONFIG_HAVE_BUILTIN_CTZ` may be better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r903982091
##########
include/endian.h:
##########
@@ -52,21 +52,50 @@
/* Common byte swapping macros */
#define __SWAP_UINT16_ISMACRO 1
-#undef __SWAP_UINT32_ISMACRO
+#define __SWAP_UINT32_ISMACRO 1
+#define __SWAP_UINT64_ISMACRO 1
Review Comment:
let's remove all __SWAP_UINTXX_ISMACRO and libs/libc/endian/lib_swapxx.c? it doesn't make sense to keep them if we always enable the macro version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#issuecomment-1164233687
@nimish please squash the change into one patch:
git rebase --interactive HEAD~9
and chang pick to sqaush except the first one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#issuecomment-1168147311
@nimish The inclusion of endian/Make.defs need be removed too:
```
Makefile:30: endian/Make.defs: No such file or directory
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] nimish commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
nimish commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r906394980
##########
include/nuttx/compiler.h:
##########
@@ -61,6 +61,14 @@
# define CONFIG_C99_BOOL 1
#endif
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
+# define CONFIG_C11_BOOL 1
+#endif
+
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201710L
+# define CONFIG_C17_BOOL 1
Review Comment:
This is for future compatibility, in conjunction with the other ones
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r905307132
##########
libs/libc/string/lib_ffsll.c:
##########
@@ -57,10 +57,12 @@ int ffsll(long long j)
if (j != 0)
{
-#ifdef CONFIG_HAVE_BUILTIN_CTZ
+#ifdef CONFIG_HAVE_BUILTIN_FFSLL
+ ret = __builtin_ffs(j);
Review Comment:
Should it be
```
ret = __builtin_ffsll(j);
```
?
##########
libs/libc/string/lib_ffsl.c:
##########
@@ -48,14 +48,15 @@
* 0, then ffsl() will return 0.
*
****************************************************************************/
-
int ffsl(long j)
{
int ret = 0;
if (j != 0)
{
-#ifdef CONFIG_HAVE_BUILTIN_CTZ
+#ifdef CONFIG_HAVE_BUILTIN_FFSL
+ ret = __builtin_ffs(j);
Review Comment:
Should it be
```
ret = __builtin_ffsl(j);
```
?
##########
include/nuttx/compiler.h:
##########
@@ -61,6 +61,14 @@
# define CONFIG_C99_BOOL 1
#endif
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
+# define CONFIG_C11_BOOL 1
Review Comment:
Where is it used?
##########
include/nuttx/compiler.h:
##########
@@ -61,6 +61,14 @@
# define CONFIG_C99_BOOL 1
#endif
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
+# define CONFIG_C11_BOOL 1
+#endif
+
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201710L
+# define CONFIG_C17_BOOL 1
Review Comment:
Where is it used?
##########
include/netinet/in.h:
##########
@@ -217,26 +217,14 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
*/
#ifdef CONFIG_ENDIAN_BIG
-# define HTONS(ns) (ns)
-# define HTONL(nl) (nl)
+# define HTONS(ns) (ns)
+# define HTONL(nl) (nl)
#else
-# define HTONS(ns) \
- (unsigned short) \
- (((((unsigned short)(ns)) & 0x00ff) << 8) | \
- ((((unsigned short)(ns)) >> 8) & 0x00ff))
-# define HTONL(nl) \
- (unsigned long) \
- (((((unsigned long)(nl)) & 0x000000ffUL) << 24) | \
- ((((unsigned long)(nl)) & 0x0000ff00UL) << 8) | \
- ((((unsigned long)(nl)) & 0x00ff0000UL) >> 8) | \
- ((((unsigned long)(nl)) & 0xff000000UL) >> 24))
+# define HTONS(ns) (__swap_uint16(ns))
+# define HTONL(nl) (__swap_uint32(nl))
Review Comment:
```suggestion
# define HTONS(ns) __swap_uint16(ns)
# define HTONL(nl) __swap_uint32(nl)
```
Or
```suggestion
# define HTONS __swap_uint16
# define HTONL __swap_uint32
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#issuecomment-1167728092
> @xiaoxiang781216 looking into CI I see some warnings like
>
> ```
> warning: (NET_TCP_NO_STACK && NET_USRSOCK_TCP) selects NET_TCP which has unmet direct dependencies (NET && SCHED_WORKQUEUE)
> ```
>
> I think that is related to one of latest reworks in TCP TX poll timer. Could you please investigate it?
I will take look tomorrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#issuecomment-1166943491
@nimish the change looks good now, but a minor style issue need be addressed before merging:
https://github.com/apache/incubator-nuttx/runs/7067463510?check_suite_focus=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r906658298
##########
include/nuttx/compiler.h:
##########
@@ -69,6 +69,36 @@
# undef CONFIG_HAVE_CXX14
#endif
+#if defined(__cplusplus) && __cplusplus >= 201703L
+# define CONFIG_HAVE_CXX17 1
+#else
+# undef CONFIG_HAVE_CXX17
+#endif
+
+
+/* Check for intrinsics */
+#ifdef __has_builtin
+# if __has_builtin(__builtin_bswap16)
+# define CONFIG_HAVE_BUILTIN_BSWAP16 1
+# endif
+# if __has_builtin(__builtin_bswap32)
+# define CONFIG_HAVE_BUILTIN_BSWAP32 1
+# endif
+# if __has_builtin(__builtin_bswap64) && defined(CONFIG_HAVE_LONG_LONG)
+# define CONFIG_HAVE_BUILTIN_BSWAP64 1
+# endif
+# if __has_builtin(__builtin_ctz)
+# define CONFIG_HAVE_BUILTIN_CTZ 1
+# endif
+# if __has_builtin(__builtin_clz)
+# define CONFIG_HAVE_BUILTIN_CLZ 1
+# endif
+# if __has_builtin(__builtin_popcount)
+# define CONFIG_HAVE_BUILTIN_POPCOUNT 1
+# endif
+#endif
Review Comment:
Yes. For Clang you can even redefine `__GNUC__` to a desired value, so with compiler option you can reach the desired functionality.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r903389057
##########
include/netinet/in.h:
##########
@@ -217,26 +217,16 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
*/
#ifdef CONFIG_ENDIAN_BIG
# define HTONS(ns) (ns)
# define HTONL(nl) (nl)
+#elif defined (CONFIG_ENDIAN_LITTLE)
+# define HTONS(ns) ((uint16_t)__swap_uint16(((uint16_t) (ns))))
+# define HTONL(nl) ((uint32_t)__swap_uint32(((uint32_t) (nl))))
Review Comment:
```suggestion
# define HTONS(ns) (__swap_uint16(ns))
# define HTONL(nl) (__swap_uint32(nl))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r903392801
##########
include/netinet/in.h:
##########
@@ -217,26 +217,16 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
Review Comment:
Why this is removed?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r901553985
##########
include/netinet/in.h:
##########
@@ -217,26 +217,30 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
*/
#ifdef CONFIG_ENDIAN_BIG
# define HTONS(ns) (ns)
# define HTONL(nl) (nl)
#else
+# ifdef __has_builtin && __has_builtin(__builtin_bswap16)
+# define HTONS(ns) ((unsigned short)__builtin_bswap16(((unsigned short) (ns))))
Review Comment:
This is purely compiler dependent, so better to move compiler stuff to `compiler..h`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#issuecomment-1167724409
@nimish please squash the second patch into the first.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] nimish commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
nimish commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r906397250
##########
libs/libc/string/lib_ffsll.c:
##########
@@ -57,10 +57,12 @@ int ffsll(long long j)
if (j != 0)
{
-#ifdef CONFIG_HAVE_BUILTIN_CTZ
+#ifdef CONFIG_HAVE_BUILTIN_FFSLL
+ ret = __builtin_ffs(j);
Review Comment:
yep
##########
libs/libc/string/lib_ffsl.c:
##########
@@ -48,14 +48,15 @@
* 0, then ffsl() will return 0.
*
****************************************************************************/
-
int ffsl(long j)
{
int ret = 0;
if (j != 0)
{
-#ifdef CONFIG_HAVE_BUILTIN_CTZ
+#ifdef CONFIG_HAVE_BUILTIN_FFSL
+ ret = __builtin_ffs(j);
Review Comment:
yep
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r906643219
##########
include/nuttx/compiler.h:
##########
@@ -69,10 +77,30 @@
# undef CONFIG_HAVE_CXX14
#endif
+#if defined(__cplusplus) && __cplusplus >= 201703L
Review Comment:
move to other patch
##########
include/endian.h:
##########
@@ -51,23 +51,41 @@
/* Common byte swapping macros */
-#define __SWAP_UINT16_ISMACRO 1
-#undef __SWAP_UINT32_ISMACRO
-
-#ifdef __SWAP_UINT16_ISMACRO
+#ifdef CONFIG_HAVE_BUILTIN_BSWAP16
+# define __swap_uint16 __builtin_bswap16
+#else
# define __swap_uint16(n) \
(uint16_t)(((((uint16_t)(n)) & 0x00ff) << 8) | \
((((uint16_t)(n)) >> 8) & 0x00ff))
#endif
-#ifdef __SWAP_UINT32_ISMACRO
+#ifdef CONFIG_HAVE_BUILTIN_BSWAP32
+# define __swap_uint32 __builtin_bswap32
+#else
# define __swap_uint32(n) \
(uint32_t)(((((uint32_t)(n)) & 0x000000ffUL) << 24) | \
((((uint32_t)(n)) & 0x0000ff00UL) << 8) | \
((((uint32_t)(n)) & 0x00ff0000UL) >> 8) | \
((((uint32_t)(n)) & 0xff000000UL) >> 24))
#endif
+#ifdef CONFIG_HAVE_LONG_LONG
+#ifdef CONFIG_HAVE_BUILTIN_BSWAP64
Review Comment:
let's indent the inner #ifdef/#else/#endif by two spaces
##########
include/nuttx/compiler.h:
##########
@@ -61,6 +61,14 @@
# define CONFIG_C99_BOOL 1
#endif
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L
+# define CONFIG_C11_BOOL 1
+#endif
+
+#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201710L
+# define CONFIG_C17_BOOL 1
Review Comment:
it's better to create a new patch and PR instead
##########
libs/libc/string/lib_ffsl.c:
##########
@@ -48,14 +48,15 @@
* 0, then ffsl() will return 0.
*
****************************************************************************/
-
Review Comment:
revert
##########
include/netinet/in.h:
##########
@@ -217,26 +217,14 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
*/
#ifdef CONFIG_ENDIAN_BIG
-# define HTONS(ns) (ns)
-# define HTONL(nl) (nl)
+# define HTONS(ns) (ns)
+# define HTONL(nl) (nl)
#else
-# define HTONS(ns) \
- (unsigned short) \
- (((((unsigned short)(ns)) & 0x00ff) << 8) | \
- ((((unsigned short)(ns)) >> 8) & 0x00ff))
-# define HTONL(nl) \
- (unsigned long) \
- (((((unsigned long)(nl)) & 0x000000ffUL) << 24) | \
- ((((unsigned long)(nl)) & 0x0000ff00UL) << 8) | \
- ((((unsigned long)(nl)) & 0x00ff0000UL) >> 8) | \
- ((((unsigned long)(nl)) & 0xff000000UL) >> 24))
+# define HTONS __swap_uint16
Review Comment:
need include endian.h at the beginning.
##########
libs/libc/string/lib_ffs.c:
##########
@@ -48,14 +48,15 @@
* 0, then ffs() will return 0.
*
****************************************************************************/
-
Review Comment:
revert the change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] nimish closed pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
nimish closed pull request #6482: Use builtins for byteswapping
URL: https://github.com/apache/incubator-nuttx/pull/6482
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r903381099
##########
include/endian.h:
##########
@@ -52,21 +52,50 @@
/* Common byte swapping macros */
#define __SWAP_UINT16_ISMACRO 1
-#undef __SWAP_UINT32_ISMACRO
+#define __SWAP_UINT32_ISMACRO 1
+#define __SWAP_UINT64_ISMACRO 1
+
#ifdef __SWAP_UINT16_ISMACRO
+#ifdef CONFIG_HAVE_BUILTIN_BSWAP16
Review Comment:
```suggestion
# ifdef CONFIG_HAVE_BUILTIN_BSWAP16
```
##########
include/endian.h:
##########
@@ -52,21 +52,50 @@
/* Common byte swapping macros */
#define __SWAP_UINT16_ISMACRO 1
-#undef __SWAP_UINT32_ISMACRO
+#define __SWAP_UINT32_ISMACRO 1
+#define __SWAP_UINT64_ISMACRO 1
+
#ifdef __SWAP_UINT16_ISMACRO
+#ifdef CONFIG_HAVE_BUILTIN_BSWAP16
+# define __swap_uint16(n) ((uint16_t)__builtin_bswap16((uint32_t)(n)))
+#else
# define __swap_uint16(n) \
Review Comment:
```suggestion
# define __swap_uint16(n) \
```
##########
include/endian.h:
##########
@@ -52,21 +52,50 @@
/* Common byte swapping macros */
#define __SWAP_UINT16_ISMACRO 1
-#undef __SWAP_UINT32_ISMACRO
+#define __SWAP_UINT32_ISMACRO 1
+#define __SWAP_UINT64_ISMACRO 1
+
#ifdef __SWAP_UINT16_ISMACRO
+#ifdef CONFIG_HAVE_BUILTIN_BSWAP16
+# define __swap_uint16(n) ((uint16_t)__builtin_bswap16((uint32_t)(n)))
+#else
Review Comment:
```suggestion
# else
```
##########
include/endian.h:
##########
@@ -52,21 +52,50 @@
/* Common byte swapping macros */
#define __SWAP_UINT16_ISMACRO 1
-#undef __SWAP_UINT32_ISMACRO
+#define __SWAP_UINT32_ISMACRO 1
+#define __SWAP_UINT64_ISMACRO 1
+
#ifdef __SWAP_UINT16_ISMACRO
+#ifdef CONFIG_HAVE_BUILTIN_BSWAP16
+# define __swap_uint16(n) ((uint16_t)__builtin_bswap16((uint32_t)(n)))
+#else
# define __swap_uint16(n) \
(uint16_t)(((((uint16_t)(n)) & 0x00ff) << 8) | \
((((uint16_t)(n)) >> 8) & 0x00ff))
#endif
Review Comment:
```suggestion
# endif
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r903389057
##########
include/netinet/in.h:
##########
@@ -217,26 +217,16 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
*/
#ifdef CONFIG_ENDIAN_BIG
# define HTONS(ns) (ns)
# define HTONL(nl) (nl)
+#elif defined (CONFIG_ENDIAN_LITTLE)
+# define HTONS(ns) ((uint16_t)__swap_uint16(((uint16_t) (ns))))
+# define HTONL(nl) ((uint32_t)__swap_uint32(((uint32_t) (nl))))
Review Comment:
```suggestion
# define HTONS(ns) (__swap_uint16(ns))
# define HTONL(nl) (__swap_uint32(nl))
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] nimish commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
nimish commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r904213080
##########
include/nuttx/compiler.h:
##########
@@ -69,6 +69,36 @@
# undef CONFIG_HAVE_CXX14
#endif
+#if defined(__cplusplus) && __cplusplus >= 201703L
+# define CONFIG_HAVE_CXX17 1
+#else
+# undef CONFIG_HAVE_CXX17
+#endif
+
+
+/* Check for intrinsics */
+#ifdef __has_builtin
+# if __has_builtin(__builtin_bswap16)
+# define CONFIG_HAVE_BUILTIN_BSWAP16 1
+# endif
+# if __has_builtin(__builtin_bswap32)
+# define CONFIG_HAVE_BUILTIN_BSWAP32 1
+# endif
+# if __has_builtin(__builtin_bswap64) && defined(CONFIG_HAVE_LONG_LONG)
+# define CONFIG_HAVE_BUILTIN_BSWAP64 1
+# endif
+# if __has_builtin(__builtin_ctz)
+# define CONFIG_HAVE_BUILTIN_CTZ 1
+# endif
+# if __has_builtin(__builtin_clz)
+# define CONFIG_HAVE_BUILTIN_CLZ 1
+# endif
+# if __has_builtin(__builtin_popcount)
+# define CONFIG_HAVE_BUILTIN_POPCOUNT 1
+# endif
+#endif
Review Comment:
I just switched these to checking the symbol itself, since `__GNUC__` can be disabled on clang but the builtins might still exist.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] nimish commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
nimish commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r903220886
##########
include/netinet/in.h:
##########
@@ -217,26 +217,30 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
*/
#ifdef CONFIG_ENDIAN_BIG
# define HTONS(ns) (ns)
# define HTONL(nl) (nl)
#else
+# ifdef __has_builtin && __has_builtin(__builtin_bswap16)
+# define HTONS(ns) ((unsigned short)__builtin_bswap16(((unsigned short) (ns))))
Review Comment:
Done, moved to other headers
##########
include/netinet/in.h:
##########
@@ -232,7 +232,7 @@
((((unsigned short)(ns)) >> 8) & 0x00ff))
# endif
# ifdef __has_builtin && __has_builtin(__builtin_bswap32)
-# define HTONL(nl) ((unsigned long)__builtin_bswap32(((unsigned short) (nl))))
+# define HTONL(nl) ((unsigned long)__builtin_bswap32(((unsigned long) (nl))))
Review Comment:
done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] xiaoxiang781216 commented on a diff in pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#discussion_r901214743
##########
include/netinet/in.h:
##########
@@ -217,26 +217,30 @@
/* This macro to convert a 16/32-bit constant values quantity from host byte
* order to network byte order. The 16-bit version of this macro is required
* for uIP:
- *
- * Author Adam Dunkels <ad...@dunkels.com>
- * Copyright (c) 2001-2003, Adam Dunkels.
- * All rights reserved.
*/
#ifdef CONFIG_ENDIAN_BIG
# define HTONS(ns) (ns)
# define HTONL(nl) (nl)
#else
+# ifdef __has_builtin && __has_builtin(__builtin_bswap16)
+# define HTONS(ns) ((unsigned short)__builtin_bswap16(((unsigned short) (ns))))
Review Comment:
it's better to update the related macros in endian.h and forward HTONS to them.
##########
include/netinet/in.h:
##########
@@ -232,7 +232,7 @@
((((unsigned short)(ns)) >> 8) & 0x00ff))
# endif
# ifdef __has_builtin && __has_builtin(__builtin_bswap32)
-# define HTONL(nl) ((unsigned long)__builtin_bswap32(((unsigned short) (nl))))
+# define HTONL(nl) ((unsigned long)__builtin_bswap32(((unsigned long) (nl))))
Review Comment:
merge to previous patch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] nimish commented on pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
nimish commented on PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#issuecomment-1166848264
bizarre. no idea why this closed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #6482: Use builtins for byteswapping
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on PR #6482:
URL: https://github.com/apache/incubator-nuttx/pull/6482#issuecomment-1166978738
@xiaoxiang781216 looking into CI I see some warnings like
```
warning: (NET_TCP_NO_STACK && NET_USRSOCK_TCP) selects NET_TCP which has unmet direct dependencies (NET && SCHED_WORKQUEUE)
```
I think that is related to one of latest reworks in TCP TX poll timer. Could you please investigate it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org