Re: [PATCH v3 04/18] Add string vectorized find and detection functions

[Adhemerval Zanella <adhemerval dot zanella at linaro dot org>]

On 11/01/2018 11:34, Joseph Myers wrote:
> On Wed, 10 Jan 2018, Adhemerval Zanella wrote:
> 
>> +
>> +static inline unsigned char
>> +extractbyte (op_t x, unsigned idx)
> 
> Missing comment on the semantics of this function.  "unsigned int".

I applied this change locally:

diff --git a/sysdeps/generic/string-extbyte.h b/sysdeps/generic/string-extbyte.h
index 69a78ce..c4693b2 100644
--- a/sysdeps/generic/string-extbyte.h
+++ b/sysdeps/generic/string-extbyte.h
@@ -23,8 +23,10 @@
 #include <endian.h>
 #include <string-optype.h>
 
+/* Extract the byte at index IDX from word X, with index 0 being the
+   least significant byte.  */
 static inline unsigned char
-extractbyte (op_t x, unsigned idx)
+extractbyte (op_t x, unsigned int idx)
 {
   if (__BYTE_ORDER == __LITTLE_ENDIAN)
     return x >> (idx * CHAR_BIT);


> 
>> +/* For architecture which only provides __builtin_clz{l} (HAVE_BUILTIN_CLZ)
>> +   and/or __builtin_ctz{l} (HAVE_BUILTIN_CTZ) which uses external libcalls
>> +   (for intance __c{l,t}z{s,d}i2 from libgcc) the following wrapper provides
>> +   inline implementation for both count leading zeros and count trailing
>> +   zeros using branchless computation.  */
> 
> I think the comments need to say a bit more about the semantics of these 
> functions.  In particular, do they follow the same rule as the built-in 
> functions that behavior is undefined if the argument is zero?  If they do, 
> then I'd expect the comments on the functions that call them to specify 
> that they must not be called with a zero argument (zero arguments in this 
> case generally corresponding to words that are not at the end of the 
> string etc., so the functions indeed don't get called in that case).
> 

I think it is reasonable to set the same semantic as the builtins and I
did not pay attention to outline this mainly because they are not indeed
used called in such cases (which would be UB for the builtin case in
fact).  I applied this patch locally:

diff --git a/sysdeps/generic/string-fzi.h b/sysdeps/generic/string-fzi.h
index 57101f2..534babb 100644
--- a/sysdeps/generic/string-fzi.h
+++ b/sysdeps/generic/string-fzi.h
@@ -29,7 +29,7 @@
 
    [1] https://chessprogramming.wikispaces.com/Kim+Walisch  */
 
-static inline unsigned
+static inline unsigned int
 index_access (const op_t i)
 {
   static const char index[] =
@@ -53,13 +53,14 @@ index_access (const op_t i)
   return index[i];
 }
 
-/* For architecture which only provides __builtin_clz{l} (HAVE_BUILTIN_CLZ)
+/* For architectures which only provides __builtin_clz{l} (HAVE_BUILTIN_CLZ)
    and/or __builtin_ctz{l} (HAVE_BUILTIN_CTZ) which uses external libcalls
    (for intance __c{l,t}z{s,d}i2 from libgcc) the following wrapper provides
    inline implementation for both count leading zeros and count trailing
-   zeros using branchless computation.  */
+   zeros using branchless computation.  As for builtin, if x is 0 the
+   result is undefined.*/
 
-static inline unsigned
+static inline unsigned int
 __ctz (op_t x)
 {
 #if !HAVE_BUILTIN_CTZ
@@ -78,7 +79,7 @@ __ctz (op_t x)
 #endif
 };
 
-static inline unsigned
+static inline unsigned int
 __clz (op_t x)
 {
 #if !HAVE_BUILTIN_CLZ