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 2020/02/01 08:40:51 UTC
[GitHub] [incubator-nuttx] minabeoki opened a new pull request #194:
Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
minabeoki opened a new pull request #194: Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
URL: https://github.com/apache/incubator-nuttx/pull/194
optimize gran_alloc() function by using __builtin_ctz().
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] minabeoki commented on issue #194: Optimize
gran_alloc() in mm_granalloc.c by using builtin ctz.
Posted by GitBox <gi...@apache.org>.
minabeoki commented on issue #194: Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
URL: https://github.com/apache/incubator-nuttx/pull/194#issuecomment-581043600
I see.
I agree with your proposal to add these special case inline functions that can be used in the common code. I can modify my code by using these mechanism.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #194:
Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #194: Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
URL: https://github.com/apache/incubator-nuttx/pull/194#issuecomment-581033848
I think another option could be to add and special case inline function to include/strings.h (strings.h, not string.h) like:
#include <nuttx/compiler.h>
#if defined(CONFIG_HAVE_BUILTIN_CTZ) && defined(CONFIG_HAVE_INLINE)
static inline int ffs(int j)
{
if (j > 0)
{
/* Count trailing zeros function can be used to implement ffs. */
return __builtin_ctz(j) + 1;
}
return 0;
}
#endif
Of course, you would also have to suppress the normal ffs() function in that case. For symmetry, the same should be done for ffsl() and ffsll().
I suppose, technically, compiler-specific code should not be standard header files either, but there are already a couple of cases like that -- for example, alloca.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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #194: Optimize
gran_alloc() in mm_granalloc.c by using builtin ctz.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #194: Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
URL: https://github.com/apache/incubator-nuttx/pull/194#issuecomment-581033848
I think another option could be to add and special case inline function to include/strings.h (strings.h, not string.h) like:
#include <nuttx/compiler.h>
#if defined(CONFIG_HAVE_BUILTIN_CTZ) && defined(CONFIG_HAVE_INLINE)
static inline int ffs(int j)
{
if (j > 0)
{
/* Count trailing zeros function can be used to implement ffs. */
return __builtin_ctz(j) + 1;
}
return 0;
}
#endif
Of course, you would also have to suppress the normal ffs() function in that case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #194: Optimize
gran_alloc() in mm_granalloc.c by using builtin ctz.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #194: Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
URL: https://github.com/apache/incubator-nuttx/pull/194#issuecomment-581028170
You may not called GCC builtin's directly in main code. That is forbidden by the coding standard. Compiler-specific and architecture-specifc logic may never appear in common code.
You may, however, use libs/libc/strings/lib_ffs.c or lib_ffsl.c. Those will isolate the use of CTZ out of common code.
[That logic at libs/libc/strings/ is also incorrect. Those functions should be moved to libs/libc/machine. That is where architecture-specific logic belongs.]
I am going to close this for now. But if you want to update the PR using the correctrly isolated functions, it can be reopened.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #194: Optimize
gran_alloc() in mm_granalloc.c by using builtin ctz.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #194: Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
URL: https://github.com/apache/incubator-nuttx/pull/194#issuecomment-581024248
We cannot accept any compiler dependent code in the common code.
PLEASE DO NOT MERGE
I am on my cell phone now. I will make some suggestions later.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo closed pull request #194: Optimize
gran_alloc() in mm_granalloc.c by using builtin ctz.
Posted by GitBox <gi...@apache.org>.
patacongo closed pull request #194: Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
URL: https://github.com/apache/incubator-nuttx/pull/194
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo edited a comment on issue #194:
Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
Posted by GitBox <gi...@apache.org>.
patacongo edited a comment on issue #194: Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
URL: https://github.com/apache/incubator-nuttx/pull/194#issuecomment-581033848
I think another option could be to add and special case inline function to include/strings.h (strings.h, not string.h) like:
#include <nuttx/compiler.h>
#if defined(CONFIG_HAVE_BUILTIN_CTZ) && defined(CONFIG_HAVE_INLINE)
static inline int ffs(int j)
{
if (j > 0)
{
/* Count trailing zeros function can be used to implement ffs. */
return __builtin_ctz(j) + 1;
}
return 0;
}
#endif
Of course, you would also have to suppress the normal ffs() function in that case.
I suppose, technically, compiler-specific code should not be standard header files either, but there are already a couple of cases like that -- for example, alloca.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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #194:
Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #194: Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
URL: https://github.com/apache/incubator-nuttx/pull/194#discussion_r373773825
##########
File path: mm/mm_gran/mm_granalloc.c
##########
@@ -159,6 +159,15 @@ FAR void *gran_alloc(GRAN_HANDLE handle, size_t size)
break;
}
+#if CONFIG_HAVE_BUILTIN_CTZ
+
+ else if ((curr & 1) == 1)
+ {
+ shift = __builtin_ctz(~curr);
Review comment:
Why `__builtin_ctz(~curr)` and not `__builtin_clz(curr)` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #194: Optimize
gran_alloc() in mm_granalloc.c by using builtin ctz.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #194: Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
URL: https://github.com/apache/incubator-nuttx/pull/194#issuecomment-581034228
The are a couple of coding standard issues with using inline functions. Let me know. I can implement these for you.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] Ouss4 commented on a change in pull request #194:
Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
Posted by GitBox <gi...@apache.org>.
Ouss4 commented on a change in pull request #194: Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
URL: https://github.com/apache/incubator-nuttx/pull/194#discussion_r373773825
##########
File path: mm/mm_gran/mm_granalloc.c
##########
@@ -159,6 +159,15 @@ FAR void *gran_alloc(GRAN_HANDLE handle, size_t size)
break;
}
+#if CONFIG_HAVE_BUILTIN_CTZ
+
+ else if ((curr & 1) == 1)
+ {
+ shift = __builtin_ctz(~curr);
Review comment:
Why `__builtin_ctz(~curr)` and not `__builtin_clz(curr)` ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [incubator-nuttx] patacongo commented on issue #194: Optimize
gran_alloc() in mm_granalloc.c by using builtin ctz.
Posted by GitBox <gi...@apache.org>.
patacongo commented on issue #194: Optimize gran_alloc() in mm_granalloc.c by using builtin ctz.
URL: https://github.com/apache/incubator-nuttx/pull/194#issuecomment-581028561
include/nuttx/compiler.h is the standard place for hiding compiler-/architecgture-specific logic.
Another possibility could be to hide the architecture specific logic in a header file under arch/arm/include/ that is included by a another file under include/. See, for example, limits.h and inttypes.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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services