Re: [PATCH] Hex-coding optimizations using SVE on ARM.

John Naylor <johncnaylorls@gmail.com> writes:
> Okay, I added a comment. I also agree with Michael that my quick
> one-off was a bit hard to read so I've cleaned it up a bit. I plan to
> commit the attached by Friday, along with any bikeshedding that
> happens by then.

Couple of thoughts:

1. I was actually hoping for a comment on the constant's definition,
perhaps along the lines of

/*
 * The hex expansion of each possible byte value (two chars per value).
 */

2. Since "src" is defined as "const char *", I'm pretty sure that
pickier compilers will complain that

+        unsigned char usrc = *((unsigned char *) src);

results in casting away const.  Recommend

+        unsigned char usrc = *((const unsigned char *) src);

3.  I really wonder if

+        memcpy(dst, &hextbl[2 * usrc], 2);

is faster than copying the two bytes manually, along the lines of

+        *dst++ = hextbl[2 * usrc];
+        *dst++ = hextbl[2 * usrc + 1];

Compilers that inline memcpy() may arrive at the same machine code,
but why rely on the compiler to make that optimization?  If the
compiler fails to do so, an out-of-line memcpy() call will surely
be a loser.

A variant could be

+        const char *hexptr = &hextbl[2 * usrc];
+        *dst++ = hexptr[0];
+        *dst++ = hexptr[1];

but this supposes that the compiler fails to see the common
subexpression in the other formulation, which I believe
most modern compilers will see.

            regards, tom lane