935789 - Stop using prbit.h

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Joshua Cranmer [:jcranmer]]

Attachment: Part 1: Kill most uses of prbit.h

Comment 1

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2: Implement RotateLeft/RotateRight in MFBT, and kill uses of NSPR equivalents

Comment 2

[(no longer active)]

Comment on attachment 828358 [details] [diff] [review]
Part 1: Kill most uses of prbit.h

Review of attachment 828358 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/glue/pldhash.cpp
@@ +59,5 @@
>  
>  #else
>  
> +#define INCREMENT_RECURSION_LEVEL(table_)   do { } while(0)  
> +#define DECREMENT_RECURSION_LEVEL(table_)   do { } while(0)  

Nit: please remove these trailing whitespaces (and the above ones too)

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 828359 [details] [diff] [review]
Part 2: Implement RotateLeft/RotateRight in MFBT, and kill uses of NSPR equivalents

Review of attachment 828359 [details] [diff] [review]:
-----------------------------------------------------------------

There's also a JS_ROTATE32_LEFT in js/public/Utility.h, because of course there'd be.  Could you write a followup patch to kill that as well?  That also seems a good time to potentially fold in the optimizations in that macro -- if they still apply at all (perhaps MSVC has picked up the ability to detect and peephole-optimize rotate32 shift/or operations since it was written).  But this patch is fine as written, to be sure.

::: mfbt/MathAlgorithms.h
@@ +451,5 @@
>    return size_t(1) << CeilingLog2(x);
>  }
>  
> +/**
> + * Rotates the given value left by the amount of the shift width.

Maybe "Rotates the bits of" to be slightly clearer about what's happening.

@@ +457,5 @@
> +template<typename T>
> +inline T
> +RotateLeft(const T t, uint_fast8_t shift)
> +{
> +  static_assert(IsUnsigned<T>::value, "Rotates require unsigned values");

MOZ_ASSERT(shift < sizeof(T) * CHAR_BIT, "shifting too much!");

@@ +458,5 @@
> +inline T
> +RotateLeft(const T t, uint_fast8_t shift)
> +{
> +  static_assert(IsUnsigned<T>::value, "Rotates require unsigned values");
> +  return (t << shift) | (t >> (sizeof(T) * 8 - shift));

s/8/CHAR_BIT/

@@ +469,5 @@
> +inline T
> +RotateRight(const T t, uint_fast8_t shift)
> +{
> +  static_assert(IsUnsigned<T>::value, "Rotates require unsigned values");
> +  return (t >> shift) | (t << (sizeof(T) * 8 - shift));

...and all the same nits as on the other method, applied to this one.

Comment 4

[Masatoshi Kimura [:emk]]

prbits.h already has an optimization for MSVC.
https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prbit.h#143
IMO we should copy them to MFBT.

Comment 5

[Joshua Cranmer [:jcranmer]]

(In reply to Masatoshi Kimura [:emk] from comment #4)
> prbits.h already has an optimization for MSVC.
> https://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/include/prbit.h#143
> IMO we should copy them to MFBT.

I asked someone to test on IRC, and MSVC 2010 is apparently capable of reducing (x << a) | (x >> (32 - a)) into a rotate-left instruction. gcc and clang are both already capable of this, so there's no need to resort to #ifdef trickery here. And I prefer avoiding #ifdef's whenever I can get away with it.

Comment 6

[Joshua Cranmer [:jcranmer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f28e77e0a20
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c3792d389bc

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> There's also a JS_ROTATE32_LEFT in js/public/Utility.h, because of course
> there'd be.  Could you write a followup patch to kill that as well?  That
> also seems a good time to potentially fold in the optimizations in that
> macro -- if they still apply at all (perhaps MSVC has picked up the ability
> to detect and peephole-optimize rotate32 shift/or operations since it was
> written).  But this patch is fine as written, to be sure.

Filed as bug 937952.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f28e77e0a20
https://hg.mozilla.org/mozilla-central/rev/7c3792d389bc