Re: [v8-dev] optimise Math.floor(x/y) to use integer divisiion for specific divisor.... (issue 9638018)

[fschn...@chromium.org]

LGTM.

We'll land it.

http://codereview.chromium.org/9638018/

[Florian Schneider]

Unfortunately I had to revert the change because of Win32 compilation errors:

http://build.chromium.org/p/client.v8/builders/V8%20Win32/builds/3514/steps/compile/logs/stdio

[Alexandre Rames]

It seems

#include <intrin.h>

directive is missing.

msdn example use of __popcnt includes it.

http://msdn.microsoft.com/en-us/library/bb385231.aspx

It also includes it for _BitScanReverse and _BitScanForward
http://msdn.microsoft.com/en-us/library/fbxyd7zd(v=vs.80).aspx

But it seemed to work fine without it so I am not sure.

[Florian Schneider]

Maybe - at the moment I don't have a Windows machine set up to test it. @danno: Maybe someone else with a Windows setup can try this quickly?

[Alexandre Rames]

We are trying on an old windows machine we have here. I'll update you asap.

[Alexandre Rames]

__popcnt was introduced with VS2008.
Our VS2005 failed to compile for the same reason.

We acan test MSC_VER and use a manual CountSetBits for versions prior to 2008:

For MSVC++ 8.0 ,   _MSC_VER = 1400

So we can do 

#if _MSV_VER >= 1400

  // Use __popcnt

#else

  // Use hand coded CountSetBits.

#endif

I'll test this solution and upload it if it works.

Alexandre

[Florian Schneider]

Thanks! That sounds good to me.

[Alexandre Rames]

Ok that works on our machine (with correct VS2008's _MSC_VER 1500).

The patch should come soon.

[rodolph....@gmail.com]

Comments addressed.

http://codereview.chromium.org/9638018/

[fschn...@chromium.org]

Thanks. Still LGTM.

I'll re-land it

http://codereview.chromium.org/9638018/

[Alexandre Rames]

Thanks!

[Florian Schneider]

You're welcome. The Win32 builder still not happy. I'm not so familiar with our Windows buildbot configuration, so I'm fixing it for now by just avoiding the problematic compiler intrinsic (http://code.google.com/p/v8/source/detail?r=11428) It should not make a big difference, should it? 

[Vitaly Repeshko]

FYI. http://msdn.microsoft.com/en-us/library/bb385231.aspx says "If
you run code that uses this intrinsic on hardware that does not
support the popcnt instruction, the results are unpredictable."

-- Vitaly

> --
> v8-dev mailing list
> v8-...@googlegroups.com
> http://groups.google.com/group/v8-dev

[Florian Schneider]

Hm, interesting. I assume that we don't instruct g++ or msvc to generate any fancy SSE4.1 instructions anyway because V8 has to run on old non-SSE hardware - in which case the benefit of using the compiler intrinsics is probably void.

[Alexandre Rames]

Not using the intrinsic shouldn't make any difference here.

But __popcnt is the intrinsic, not the instruction. Shouldn't MS compiler automatically detect hardware support and use another solution if the popcnt isn't available.

[Vitaly Repeshko]

On Wed, Apr 25, 2012 at 1:24 AM, Alexandre Rames
<alexand...@gmail.com> wrote:
> Not using the intrinsic shouldn't make any difference here.
>
> But __popcnt is the intrinsic, not the instruction. Shouldn't MS compiler
> automatically detect hardware support and use another solution if the popcnt
> isn't available.

Maybe it should, but it's not what the MSDN page says it does. If it
did, this code code would have to check some global flag every time,
which would be more expensive than the operation itself, or require
some patching in runtime.


-- Vitaly

[Alexei Filippov]

2012/4/25 Vitaly Repeshko <vit...@chromium.org>

On Wed, Apr 25, 2012 at 1:24 AM, Alexandre Rames
<alexand...@gmail.com> wrote:
> Not using the intrinsic shouldn't make any difference here.
>
> But __popcnt is the intrinsic, not the instruction. Shouldn't MS compiler
> automatically detect hardware support and use another solution if the popcnt
> isn't available.

Maybe it should, but it's not what the MSDN page says it does. If it
did, this code code would have to check some global flag every time,
which would be more expensive than the operation itself, or require
some patching in runtime.

He probably meant compile time checking.

I used to implement some sse and avx intrinsics in Sun compiler and it also does not provide generic implementation (emulation) for instruction related intrinsics.

Btw their declarations are guarded by corresponding ifdefs in system headers, so you cannot place a call to intrinsic which is not supported by the target arch.

As for some other intrinsics (not instruction related), e.g. memcpy, strlen, compiler provides multiple implementations that make full use of available instruction set. 

Alexei

[Vitaly Repeshko]

On Wed, Apr 25, 2012 at 10:18 AM, Alexei Filippov <ale...@google.com> wrote:
>
> 2012/4/25 Vitaly Repeshko <vit...@chromium.org>
>>
>> On Wed, Apr 25, 2012 at 1:24 AM, Alexandre Rames
>> <alexand...@gmail.com> wrote:
>> > Not using the intrinsic shouldn't make any difference here.
>> >
>> > But __popcnt is the intrinsic, not the instruction. Shouldn't MS
>> > compiler
>> > automatically detect hardware support and use another solution if the
>> > popcnt
>> > isn't available.
>>
>> Maybe it should, but it's not what the MSDN page says it does. If it
>> did, this code code would have to check some global flag every time,
>> which would be more expensive than the operation itself, or require
>> some patching in runtime.
>>
> He probably meant compile time checking.

Compile time checking won't work because this code is compiled only
once and then used on different platforms, including old ones. E.g.,
there is no SSE2 build of V8; whether processor supports SSE2 is
checked in runtime before JIT code is compiled.


-- Vitaly

[Alexandre Rames]

I did mean compile time checking.

I agree the msdn page specifies that support for the popcnt instruction should be checked before using the corresponding intrinsic.

I did not realize that the behaviour differs here because we are using an intrinsic, not a builtin like we do for GCC. I agree the compiler should not try to emulate it, even though it would not break porting compatibility.

Cheers!

Alexandre