From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:45158) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UJ8BJ-000484-IY for qemu-devel@nongnu.org; Fri, 22 Mar 2013 16:03:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UJ8BE-0000Ex-UJ for qemu-devel@nongnu.org; Fri, 22 Mar 2013 16:03:17 -0400 Received: from mx.ipv6.kamp.de ([2a02:248:0:51::16]:39272 helo=mx01.kamp.de) by eggs.gnu.org with smtp (Exim 4.71) (envelope-from ) id 1UJ8BE-0000EY-LP for qemu-devel@nongnu.org; Fri, 22 Mar 2013 16:03:12 -0400 Message-ID: <514CB8FD.2040007@kamp.de> Date: Fri, 22 Mar 2013 21:03:09 +0100 From: Peter Lieven MIME-Version: 1.0 References: <1363956370-23681-1-git-send-email-pl@kamp.de> <1363956370-23681-3-git-send-email-pl@kamp.de> <514CB312.4090203@redhat.com> In-Reply-To: <514CB312.4090203@redhat.com> Content-Type: multipart/alternative; boundary="------------060109070905060208050204" Subject: Re: [Qemu-devel] [PATCHv4 2/9] cutils: add a function to find non-zero content in a buffer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: Paolo Bonzini , quintela@redhat.com, Orit Wasserman , qemu-devel@nongnu.org, Stefan Hajnoczi This is a multi-part message in MIME format. --------------060109070905060208050204 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Am 22.03.2013 20:37, schrieb Eric Blake: > On 03/22/2013 06:46 AM, Peter Lieven wrote: >> this adds buffer_find_nonzero_offset() which is a SSE2/Altivec >> optimized function that searches for non-zero content in a >> buffer. >> >> due to the optimizations used in the function there are restrictions >> on buffer address and search length. the function >> can_use_buffer_find_nonzero_content() can be used to check if >> the function can be used safely. >> >> Signed-off-by: Peter Lieven >> --- >> include/qemu-common.h | 13 +++++++++++++ >> util/cutils.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 58 insertions(+) >> +#define BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR 8 >> +static inline bool >> +can_use_buffer_find_nonzero_offset(const void *buf, size_t len) >> +{ >> + if (len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR >> + * sizeof(VECTYPE)) == 0 >> + && ((uintptr_t) buf) % sizeof(VECTYPE) == 0) { > I know that emacs tends to indent the second line to the column after > the ( that it is associated with, as in: > > + if (len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR > + * sizeof(VECTYPE)) == 0 > + && ((uintptr_t) buf) % sizeof(VECTYPE) == 0) { > > But since checkpatch.pl didn't complain, and since I'm not sure if there > is a codified qemu indentation style, and since I _am_ sure that not > everyone uses emacs [hi, vi guys], it's not worth respinning. A > maintainer can touch it up if desired. Actually, I was totally unsure how to indent this. Maybe just give a hint what you would like to see. As I will replace patch 4 with an earlier version that is not vector optimized, but uses loop unrolling, I will have to do a v5 and so I can fix this. > >> + >> +size_t buffer_find_nonzero_offset(const void *buf, size_t len) >> +{ >> + VECTYPE *p = (VECTYPE *)buf; >> + VECTYPE zero = ZERO_SPLAT; >> + size_t i; >> + >> + assert(len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR >> + * sizeof(VECTYPE)) == 0); >> + assert(((uintptr_t) buf) % sizeof(VECTYPE) == 0); > I would have written this: > > assert(can_use_buffer_find_nonzero_offset(buf, len)); Good point. Will be changed in v5. > But that's cosmetic, and compiles to the same code, so it's not worth a > respin. > > You've addressed my concerns on v3. > > Reviewed-by: Eric Blake > Peter --------------060109070905060208050204 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit 
Am 22.03.2013 20:37, schrieb Eric Blake:

On 03/22/2013 06:46 AM, Peter Lieven wrote:

this adds buffer_find_nonzero_offset() which is a SSE2/Altivec
optimized function that searches for non-zero content in a
buffer.

due to the optimizations used in the function there are restrictions
on buffer address and search length. the function
can_use_buffer_find_nonzero_content() can be used to check if
the function can be used safely.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 include/qemu-common.h |   13 +++++++++++++
 util/cutils.c         |   45 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+)
+#define BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR 8
+static inline bool
+can_use_buffer_find_nonzero_offset(const void *buf, size_t len)
+{
+    if (len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR
+                * sizeof(VECTYPE)) == 0
+            && ((uintptr_t) buf) % sizeof(VECTYPE) == 0) {

I know that emacs tends to indent the second line to the column after
the ( that it is associated with, as in:

+    if (len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR
+               * sizeof(VECTYPE)) == 0
+        && ((uintptr_t) buf) % sizeof(VECTYPE) == 0) {

But since checkpatch.pl didn't complain, and since I'm not sure if there
is a codified qemu indentation style, and since I _am_ sure that not
everyone uses emacs [hi, vi guys], it's not worth respinning.  A
maintainer can touch it up if desired.

Actually, I was totally unsure how to indent this. Maybe just give
a hint what you would like to see. As I will replace patch 4 with
an earlier version that is not vector optimized, but uses loop unrolling,
I will have to do a v5 and so I can fix this.




+
+size_t buffer_find_nonzero_offset(const void *buf, size_t len)
+{
+    VECTYPE *p = (VECTYPE *)buf;
+    VECTYPE zero = ZERO_SPLAT;
+    size_t i;
+
+    assert(len % (BUFFER_FIND_NONZERO_OFFSET_UNROLL_FACTOR
+        * sizeof(VECTYPE)) == 0);
+    assert(((uintptr_t) buf) % sizeof(VECTYPE) == 0);

I would have written this:

assert(can_use_buffer_find_nonzero_offset(buf, len));

Good point. Will be changed in v5.

But that's cosmetic, and compiles to the same code, so it's not worth a
respin.

You've addressed my concerns on v3.

Reviewed-by: Eric Blake <eblake@redhat.com>


Peter
--------------060109070905060208050204--