Bug 197816 – bitCount() should use popcount where available

• Component: Web Template Framework
• Status: NEW

[Robin Morisset]

...

[Robin Morisset]

Created attachment 369653 [details]
Patch

[EWS Watchlist]

Comment on attachment 369653 [details]
Patch

Attachment 369653 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12164598

New failing tests:
js/dom/custom-constructors.html

[EWS Watchlist]

Created attachment 369663 [details]
Archive of layout-test-results from ews212 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews212  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[Sam Weinig]

Comment on attachment 369653 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=369653&action=review

> Source/WTF/ChangeLog:9
> +        * wtf/StdLibExtras.h:
> +        (WTF::bitCount):

Do we have any tests for bitCount? Might be a good time to add one, make sure there is no weirdness around the edge cases (e.g. 0 and MAX_UINT).

> Source/WTF/wtf/StdLibExtras.h:157
> +#if COMPILER(GCC_COMPATIBLE)

The preferred way to do this would be to add a WTF_COMPILER_SUPPORTS_BUILTIN_POPCOUNT to Compiler.h, and then do a COMPILER_SUPPORTS(BUILTIN_POPCOUNT) check here. That way, if we ever change/add compilers, there is one place were we can define it's feature set.

[Daniel Bates]

I could have sworn a group of us came up with a slightly more complete and maybe even optimal? impl. (Surprising right?) here: <https://bugs.webkit.org/attachment.cgi?id=350281&action=prettypatch> (bug# 189633). But I can’t remember if we proved it by dumping assembly. Can we use ^ that code?

[Daniel Bates]

Eh, maybe the existing branch less one is still better when you don’t have the instruction. I don’t know.

[Robin Morisset]

(In reply to Daniel Bates from comment #5)
> I could have sworn a group of us came up with a slightly more complete and
> maybe even optimal? impl. (Surprising right?) here:
> <https://bugs.webkit.org/attachment.cgi?id=350281&action=prettypatch> (bug#
> 189633). But I can’t remember if we proved it by dumping assembly. Can we
> use ^ that code?

Thanks for the pointer, I've reviewed that patch.

I agree that these two patches are trying to do the same thing and should be merged.

[Robin Morisset]

(In reply to Sam Weinig from comment #4)
> Comment on attachment 369653 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369653&action=review
> 
> > Source/WTF/ChangeLog:9
> > +        * wtf/StdLibExtras.h:
> > +        (WTF::bitCount):
> 
> Do we have any tests for bitCount? Might be a good time to add one, make
> sure there is no weirdness around the edge cases (e.g. 0 and MAX_UINT).

I will do that.

> 
> > Source/WTF/wtf/StdLibExtras.h:157
> > +#if COMPILER(GCC_COMPATIBLE)
> 
> The preferred way to do this would be to add a
> WTF_COMPILER_SUPPORTS_BUILTIN_POPCOUNT to Compiler.h, and then do a
> COMPILER_SUPPORTS(BUILTIN_POPCOUNT) check here. That way, if we ever
> change/add compilers, there is one place were we can define it's feature set.

A difficulty here is that different compilers can have different syntaxes for builtins (e.g. __builtin_popcntll for Clang/GCC vs __popcnt64 for MSVC). So I am tempted to keep checking for specific compilers here. But at least I should replace GCC_COMPATIBLE (which is not what I thought) by GCC_OR_CLANG.

I thought of supporting MSVC, but it is tricky because its __popcnt builtin is only valid on machines with the corresponding instruction (see my comment on https://bugs.webkit.org/show_bug.cgi?id=189633)

[Robin Morisset]

Created attachment 369791 [details]
Patch

Added a test, and replaced GCC_COMPATIBLE by GCC_OR_CLANG.

[Sam Weinig]

(In reply to Robin Morisset from comment #8)
> (In reply to Sam Weinig from comment #4)
> > 
> > > Source/WTF/wtf/StdLibExtras.h:157
> > > +#if COMPILER(GCC_COMPATIBLE)
> > 
> > The preferred way to do this would be to add a
> > WTF_COMPILER_SUPPORTS_BUILTIN_POPCOUNT to Compiler.h, and then do a
> > COMPILER_SUPPORTS(BUILTIN_POPCOUNT) check here. That way, if we ever
> > change/add compilers, there is one place were we can define it's feature set.
> 
> A difficulty here is that different compilers can have different syntaxes
> for builtins (e.g. __builtin_popcntll for Clang/GCC vs __popcnt64 for MSVC).

I don't think that is a problem. Just make the defines WTF_COMPILER_SUPPORTS_BUILTIN_POPCOUNT/WTF_COMPILER_SUPPORTS_BUILTIN_POPCOUNTLL. Since the MSVC one is called __popcnt64, it won't be defined for MSVC.

[Yusuke Suzuki]

Comment on attachment 369791 [details]
Patch

Looks like build is failing.