1022049 - Disable bitscan intrinsics on clang-cl because it doesn't support #pragma intrinsic

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Comment 1

[(no longer active)]

Attachment: Disable bitscan intrinsics on clang-cl because it doesn't support #pragma intrinsic

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Can we just #include <intrinsic.h> instead? We'll need to do that for the atomic intrinsics anyways.

Comment 3

[(no longer active)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> Can we just #include <intrinsic.h> instead? We'll need to do that for the
> atomic intrinsics anyways.

Will that fix the error?  I wrote this patch according to our local workaround.

Comment 4

[(no longer active)]

Attachment: #include intrin.h directly instead of defining the intrinsics manually; r=froydnj

Comment 5

[(no longer active)]

Comment on attachment 8436194 [details] [diff] [review]
Disable bitscan intrinsics on clang-cl because it doesn't support #pragma intrinsic

Jeff is right!

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8436564 [details] [diff] [review]
#include intrin.h directly instead of defining the intrinsics manually; r=froydnj

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

I don't understand how Jeff is right here; we manually prototype the Atomics.h stuff already?

::: mfbt/MathAlgorithms.h
@@ +146,5 @@
>  } // namespace mozilla
>  
>  #if defined(_WIN32) && (_MSC_VER >= 1300) && (defined(_M_IX86) || defined(_M_AMD64) || defined(_M_X64))
>  #  define MOZ_BITSCAN_WINDOWS
> +#  include <intrin.h>

Can you provide some sort of idea of how many headers this pulls in?  For instance, we're not dragging in windows.h just for this, are we?

For the Atomics.h stuff, we just copied in our own prototypes rather than #include'ing what we need, in an effort to keep these headers light.  I suppose the same motivations were present here.

Comment 7

[(no longer active)]

Comment on attachment 8436564 [details] [diff] [review]
#include intrin.h directly instead of defining the intrinsics manually; r=froydnj

This pulls in only 25 headers, some of which are already popular ones, such as malloc.h, stddef.h (and the list doesn't include windows.h.

I find it really surprising to micro-optimize not pulling in this header and provide our own definitions.  Also note that this code was added in bug 356856, and I doubt there was a concern of the size of the headers there at all.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8436564 [details] [diff] [review]
#include intrin.h directly instead of defining the intrinsics manually; r=froydnj

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

Well, clearly in 2006, people were adding their own prototypes for whatever reasons, so...

Unless I'm missing something, intrin.h doesn't #pragma intrinsic anything, so we'll get out-of-line function calls for what were previously inlined code...right?

Comment 9

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8436564 [details] [diff] [review]
#include intrin.h directly instead of defining the intrinsics manually; r=froydnj

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

::: mfbt/MathAlgorithms.h
@@ +146,5 @@
>  } // namespace mozilla
>  
>  #if defined(_WIN32) && (_MSC_VER >= 1300) && (defined(_M_IX86) || defined(_M_AMD64) || defined(_M_X64))
>  #  define MOZ_BITSCAN_WINDOWS
> +#  include <intrin.h>

If we need to we could only #include <intrin.h> with clang and leave our own definitions for msvc.

@@ -156,5 @@
> -#  if defined(_M_AMD64) || defined(_M_X64)
> -#    define MOZ_BITSCAN_WINDOWS64
> -    unsigned char _BitScanForward64(unsigned long* index, unsigned __int64 mask);
> -    unsigned char _BitScanReverse64(unsigned long* index, unsigned __int64 mask);
> -#   pragma intrinsic(_BitScanForward64, _BitScanReverse64)

I believe the pragma intrinsic(...) should stay. clang ignores it.

Comment 10

[(no longer active)]

Hmm, so I reverted the patch and clang-cl seems to build MathAlgorithms.h quite happily without it.  Not sure what problem we were hitting before but it doesn't seem to exist any more.

Comment 11

[(no longer active)]

For those following along at home, Jeff told me that the error we were getting was during linking, which explains why I did not see it in comment 10.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Attachment: #include intrin.h directly instead of defining the intrinsics manually; r=froydnj

This just removes the manual definitions and includes <intrin.h>. The pragmas are left. This builds fine on windows https://tbpl.mozilla.org/?tree=Try&rev=af950949b678 and is more correct than manually defining these functions ourselves.

Comment 13

[Nathan Froyd [:froydnj]]

Comment on attachment 8437441 [details] [diff] [review]
#include intrin.h directly instead of defining the intrinsics manually; r=froydnj

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

r=me with the change below.

::: mfbt/MathAlgorithms.h
@@ +152,1 @@
>    extern "C" {

Please remove the extern "C" wrapper; it doesn't seem to be needed for the pragmas (cf. similar code in Atomics.h).

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e19324807568