Обсуждение: use rotate macro in more places

We've accumulated a few bit-twiddling hacks to get the compiler to
emit a rotate instruction. Since we have a macro for that, let's use
it, as in the attached. I thought the new call sites would look better
with a "left" version, so I added a new macro for that. That's not
necessary, however.

Some comments now look a bit too obvious to keep around, but maybe
they should be replaced with a "why", instead of a "what":

                        /* rotate hashkey left 1 bit at each step */
-                       hashkey = (hashkey << 1) | ((hashkey &
0x80000000) ? 1 : 0);
+                       hashkey = pg_rotate_left32(hashkey, 1);


-- 
John Naylor
EDB: http://www.enterprisedb.com

John Naylor <john.naylor@enterprisedb.com> writes:
> We've accumulated a few bit-twiddling hacks to get the compiler to
> emit a rotate instruction. Since we have a macro for that, let's use
> it, as in the attached. I thought the new call sites would look better
> with a "left" version, so I added a new macro for that. That's not
> necessary, however.

+1 --- I didn't look to see if you missed any spots, but this is
good as far as it goes.

> Some comments now look a bit too obvious to keep around, but maybe
> they should be replaced with a "why", instead of a "what":

Yeah.  Maybe like "combine successive hashkeys by rotating"?

            regards, tom lane

On Sat, 19 Feb 2022 20:07:58 +0700
John Naylor <john.naylor@enterprisedb.com> wrote:

> We've accumulated a few bit-twiddling hacks to get the compiler to
> emit a rotate instruction. Since we have a macro for that, let's use
> it, as in the attached. I thought the new call sites would look better
> with a "left" version, so I added a new macro for that. That's not
> necessary, however.
> 
> Some comments now look a bit too obvious to keep around, but maybe
> they should be replaced with a "why", instead of a "what":
> 
>                         /* rotate hashkey left 1 bit at each step */
> -                       hashkey = (hashkey << 1) | ((hashkey &
> 0x80000000) ? 1 : 0);
> +                       hashkey = pg_rotate_left32(hashkey, 1);

I think we can use this macro also in hash_multirange, hash_range, 
and JsonbHashScalarValue as in the attached patch. How about replacing
them with the macro, too.

For avoiding undefined behaviours,  maybe it is better to use unsigned
int and bit mask as a following code in Linux does[1][2], though it
would be unnecessary if they are used properly as in the current
PostgreSQL code.

 static inline __u32 rol32(__u32 word, unsigned int shift)
 {
     return (word << (shift & 31)) | (word >> ((-shift) & 31));
 }

[1] https://github.com/torvalds/linux/blob/master/include/linux/bitops.h
[2] https://lore.kernel.org/lkml/20190609164129.126354143@linuxfoundation.org/

Regards,
Yugo Nagata

> 
> 
> -- 
> John Naylor
> EDB: http://www.enterprisedb.com


-- 
Yugo NAGATA <nagata@sraoss.co.jp>

Yugo NAGATA <nagata@sraoss.co.jp> writes:
> For avoiding undefined behaviours,  maybe it is better to use unsigned
> int and bit mask as a following code in Linux does[1][2], though it
> would be unnecessary if they are used properly as in the current
> PostgreSQL code.

I don't think that's an improvement.  It would mask buggy calls.

            regards, tom lane

On Sun, Feb 20, 2022 at 1:03 AM Yugo NAGATA <nagata@sraoss.co.jp> wrote:
> I think we can use this macro also in hash_multirange, hash_range,
> and JsonbHashScalarValue as in the attached patch. How about replacing
> them with the macro, too.

Good find. I also found one more in hashfn.c.

On Sat, Feb 19, 2022 at 11:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Some comments now look a bit too obvious to keep around, but maybe
> > they should be replaced with a "why", instead of a "what":
>
> Yeah.  Maybe like "combine successive hashkeys by rotating"?

Done that way.

Pushed, thank you both for looking!
-- 
John Naylor
EDB: http://www.enterprisedb.com