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

John Naylor <johncnaylorls(at)gmail(dot)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