Fix JIT use of BitScan* APIs (#84915)

* Fix JIT use of BitScan* APIs

windows.h defines the BitScan* APIs with a leading underscore
and includes `#define` definitions of non-underscore versions
to the underscore versions.

This is annoying in the JIT, where we have a BitOperations class that
uses these same names; the class members end up with leading underscores
and it confuses Visual Studio source browsing.

In the JIT, `#undef` the windows.h underscore defines. Define pass-through
non-underscore functions to call the actual functions. (We need to always
call the non-underscore versions because that is what is defined in the PAL.)
Replace usage of bitposition.h in the JIT and remove it from utilcode.h
(only one other place in the CLR uses it and they already include it.)

* Fixes

1. Remove unused genFindHighestBit
2. Remove genFindLowestReg
3. Remove BitScanForwardPtr
4. Put BitScanForward64/BitScanReverse64 under `HOST_64BIT`

diff --git a/src/coreclr/inc/utilcode.h b/src/coreclr/inc/utilcode.h
index 1b39056d057fd0037ba5ab917b3311dbe9d89d46..bbfac6e210e92d9c52066e1ff5f04f27948795b7 100644 (file)
--- a/src/coreclr/inc/utilcode.h
+++ b/src/coreclr/inc/utilcode.h
@@ -872,8 +872,6 @@ inline int CountBits(int iNum)
     return (iBits);
 }
 
-#include "bitposition.h"
-
 // Convert the currency to a decimal and canonicalize.
 inline void VarDecFromCyCanonicalize(CY cyIn, DECIMAL* dec)
 {

diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp
index 09b2d3e7f292f055f00cea16542a9779ad2a0ca7..8215b4e1b56a8bcc4d2a93f11ffd412926dcd1b9 100644 (file)
--- a/src/coreclr/jit/compiler.cpp
+++ b/src/coreclr/jit/compiler.cpp
@@ -26,14 +26,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 
 extern ICorJitHost* g_jitHost;
 
-#if defined(DEBUG)
-// Column settings for DOTNET_JitDumpIR.  We could(should) make these programmable.
-#define COLUMN_OPCODE 30
-#define COLUMN_OPERANDS (COLUMN_OPCODE + 25)
-#define COLUMN_KINDS 110
-#define COLUMN_FLAGS (COLUMN_KINDS + 32)
-#endif
-
 unsigned Compiler::jitTotalMethodCompiled = 0;
 
 #if defined(DEBUG)

diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp
index 4957a1c30e65eea6af09b617e388bbb9616f3ca6..7e4eb4c14d895598a9caafd41f2a10829c964db4 100644 (file)
--- a/src/coreclr/jit/compiler.hpp
+++ b/src/coreclr/jit/compiler.hpp
@@ -97,53 +97,9 @@ inline T genFindLowestBit(T value)
     return (value & (0 - value));
 }
 
-//------------------------------------------------------------------------
-// genFindHighestBit:  Return the highest bit that is set (that is, a mask that includes just the
-//                     highest bit).
-//
-// Return Value:
-//    The highest position (0 is LSB) of bit that is set in the 'value'.
-//
-// Note:
-//    It performs the "LeadingZeroCount " operation using intrinsics and then mask out everything
-//    but the highest bit.
-inline unsigned int genFindHighestBit(unsigned int mask)
-{
-    assert(mask != 0);
-#if defined(_MSC_VER)
-    unsigned long index;
-#else
-    unsigned int index;
-#endif
-    BitScanReverse(&index, mask);
-    return 1L << index;
-}
-
-//------------------------------------------------------------------------
-// genFindHighestBit:  Return the highest bit that is set (that is, a mask that includes just the
-//                     highest bit).
-//
-// Return Value:
-//    The highest position (0 is LSB) of bit that is set in the 'value'.
-//
-// Note:
-//    It performs the "LeadingZeroCount " operation using intrinsics and then mask out everything
-//    but the highest bit.
-inline unsigned __int64 genFindHighestBit(unsigned __int64 mask)
-{
-    assert(mask != 0);
-#if defined(_MSC_VER)
-    unsigned long index;
-#else
-    unsigned int index;
-#endif
-    BitScanReverse64(&index, mask);
-    return 1LL << index;
-}
-
 /*****************************************************************************
 *
-*  Return true if the given 64-bit value has exactly zero or one bits set.
+*  Return true if the given value has exactly zero or one bits set.
 */
 
 template <typename T>
@@ -154,17 +110,7 @@ inline bool genMaxOneBit(T value)
 
 /*****************************************************************************
 *
-*  Return true if the given 32-bit value has exactly zero or one bits set.
-*/
-
-inline bool genMaxOneBit(unsigned value)
-{
-    return (value & (value - 1)) == 0;
-}
-
-/*****************************************************************************
-*
-*  Return true if the given 64-bit value has exactly one bit set.
+*  Return true if the given value has exactly one bit set.
 */
 
 template <typename T>
@@ -173,16 +119,6 @@ inline bool genExactlyOneBit(T value)
     return ((value != 0) && genMaxOneBit(value));
 }
 
-/*****************************************************************************
-*
-*  Return true if the given 32-bit value has exactly zero or one bits set.
-*/
-
-inline bool genExactlyOneBit(unsigned value)
-{
-    return ((value != 0) && genMaxOneBit(value));
-}
-
 /*****************************************************************************
  *
  *  Given a value that has exactly one bit set, return the position of that
@@ -190,47 +126,14 @@ inline bool genExactlyOneBit(unsigned value)
  */
 inline unsigned genLog2(unsigned value)
 {
-    return BitPosition(value);
-}
-
-// Given an unsigned 64-bit value, returns the lower 32-bits in unsigned format
-//
-inline unsigned ulo32(unsigned __int64 value)
-{
-    return static_cast<unsigned>(value);
-}
-
-// Given an unsigned 64-bit value, returns the upper 32-bits in unsigned format
-//
-inline unsigned uhi32(unsigned __int64 value)
-{
-    return static_cast<unsigned>(value >> 32);
+    assert(genExactlyOneBit(value));
+    return BitOperations::BitScanForward(value);
 }
 
-/*****************************************************************************
- *
- *  Given a value that has exactly one bit set, return the position of that
- *  bit, in other words return the logarithm in base 2 of the given value.
- */
-
 inline unsigned genLog2(unsigned __int64 value)
 {
-#ifdef HOST_64BIT
-    return BitPosition(value);
-#else // HOST_32BIT
-    unsigned     lo32 = ulo32(value);
-    unsigned     hi32 = uhi32(value);
-
-    if (lo32 != 0)
-    {
-        assert(hi32 == 0);
-        return genLog2(lo32);
-    }
-    else
-    {
-        return genLog2(hi32) + 32;
-    }
-#endif
+    assert(genExactlyOneBit(value));
+    return BitOperations::BitScanForward(value);
 }
 
 #ifdef __APPLE__
@@ -240,14 +143,18 @@ inline unsigned genLog2(size_t value)
 }
 #endif // __APPLE__
 
-/*****************************************************************************
- *
- *  Return the lowest bit that is set in the given register mask.
- */
+// Given an unsigned 64-bit value, returns the lower 32-bits in unsigned format
+//
+inline unsigned ulo32(unsigned __int64 value)
+{
+    return static_cast<unsigned>(value);
+}
 
-inline regMaskTP genFindLowestReg(regMaskTP value)
+// Given an unsigned 64-bit value, returns the upper 32-bits in unsigned format
+//
+inline unsigned uhi32(unsigned __int64 value)
 {
-    return (regMaskTP)genFindLowestBit(value);
+    return static_cast<unsigned>(value >> 32);
 }
 
 /*****************************************************************************

diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp
index d8295a496bf388fb267fe932bce51eb586fc2587..97de8d503606c916bd5031ca52667bae63d8861f 100644 (file)
--- a/src/coreclr/jit/emit.cpp
+++ b/src/coreclr/jit/emit.cpp
@@ -6164,7 +6164,7 @@ unsigned emitter::emitCalculatePaddingForLoopAlignment(insGroup* loopHeadIG,
     if (emitComp->opts.compJitAlignLoopAdaptive)
     {
         // For adaptive, adjust the loop size depending on the alignment boundary
-        maxLoopBlocksAllowed = genLog2((unsigned)alignmentBoundary) - 1;
+        maxLoopBlocksAllowed = genLog2(alignmentBoundary) - 1;
         maxLoopSize          = alignmentBoundary * maxLoopBlocksAllowed;
     }
     else

diff --git a/src/coreclr/jit/emitarm.cpp b/src/coreclr/jit/emitarm.cpp
index 81eb8ed27c26fe7ff5b12089452010cede5c9d96..a82f37421d0dc9cd56e34215669f6741ebbf025a 100644 (file)
--- a/src/coreclr/jit/emitarm.cpp
+++ b/src/coreclr/jit/emitarm.cpp
@@ -4190,7 +4190,7 @@ void emitter::emitIns_R_ARX(
         assert(!"Please use ins_Load() to select the correct instruction");
     }
 
-    unsigned shift = genLog2((unsigned)mul);
+    unsigned shift = genLog2(mul);
 
     if ((ins == INS_lea) || emitInsIsLoad(ins))
     {

diff --git a/src/coreclr/jit/gcencode.cpp b/src/coreclr/jit/gcencode.cpp
index dcbfd1d7064b0d252c53692722b0ac8e1255f293..eaa5a584261a37b109ae00fe4e262fc37b23fb5e 100644 (file)
--- a/src/coreclr/jit/gcencode.cpp
+++ b/src/coreclr/jit/gcencode.cpp
@@ -2645,7 +2645,7 @@ size_t GCInfo::gcMakeRegPtrTable(BYTE* dest, int mask, const InfoHdr& header, un
 
                     /* Get hold of the next register bit */
 
-                    tmpMask = genFindLowestReg(regMask);
+                    tmpMask = genFindLowestBit(regMask);
                     assert(tmpMask);
 
                     /* Remember the new state of this register */
@@ -2694,7 +2694,7 @@ size_t GCInfo::gcMakeRegPtrTable(BYTE* dest, int mask, const InfoHdr& header, un
 
                     /* Get hold of the next register bit */
 
-                    tmpMask = genFindLowestReg(regMask);
+                    tmpMask = genFindLowestBit(regMask);
                     assert(tmpMask);
 
                     /* Remember the new state of this register */
@@ -4627,7 +4627,7 @@ void GCInfo::gcInfoRecordGCRegStateChange(GcInfoEncoder* gcInfoEncoder,
     while (regMask)
     {
         // Get hold of the next register bit.
-        regMaskTP tmpMask = genFindLowestReg(regMask);
+        regMaskTP tmpMask = genFindLowestBit(regMask);
         assert(tmpMask);
 
         // Remember the new state of this register.

diff --git a/src/coreclr/jit/hashbv.h b/src/coreclr/jit/hashbv.h
index 1c5e4310dcfa89f4618b95c6198f9b0a8108f7a0..7ad95998add8e4537665151c51f6f843e54a24c4 100644 (file)
--- a/src/coreclr/jit/hashbv.h
+++ b/src/coreclr/jit/hashbv.h
@@ -323,12 +323,10 @@ HbvWalk ForEachHbvBitSet(const hashBv& bv, TFunctor func)
             indexType base = node->baseIndex;
             for (int el = 0; el < node->numElements(); el++)
             {
-                elemType i = 0;
                 elemType e = node->elements[el];
                 while (e)
                 {
-                    int result = BitScanForwardPtr((DWORD*)&i, e);
-                    assert(result);
+                    unsigned  i     = BitOperations::BitScanForward(e);
                     indexType index = base + (el * BITS_PER_ELEMENT) + i;
                     e ^= (elemType(1) << i);
 

diff --git a/src/coreclr/jit/jitpch.h b/src/coreclr/jit/jitpch.h
index e99ecd6c9f330354d38d063807295499021c994a..63f12133f61bffde2cad716b5626018ba026247f 100644 (file)
--- a/src/coreclr/jit/jitpch.h
+++ b/src/coreclr/jit/jitpch.h
@@ -13,6 +13,22 @@
 #include <cstdlib>
 #include <intrin.h>
 
+// Don't allow using the windows.h #defines for the BitScan* APIs. Using the #defines means our
+// `BitOperations::BitScan*` functions have their name mapped, which is confusing and messes up
+// Visual Studio source browsing.
+#ifdef BitScanForward
+#undef BitScanForward
+#endif
+#ifdef BitScanReverse
+#undef BitScanReverse
+#endif
+#ifdef BitScanForward64
+#undef BitScanForward64
+#endif
+#ifdef BitScanReverse64
+#undef BitScanReverse64
+#endif
+
 #include "jitconfig.h"
 #include "jit.h"
 #include "iallocator.h"

diff --git a/src/coreclr/jit/utils.h b/src/coreclr/jit/utils.h
index d78000bfb7d261e1018fd42c12bbccd14b1514e7..7fd3e7d10f88403e92bd955794bd7c4a72c82038 100644 (file)
--- a/src/coreclr/jit/utils.h
+++ b/src/coreclr/jit/utils.h
@@ -25,11 +25,34 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 // Needed for unreached()
 #include "error.h"
 
-#ifdef TARGET_64BIT
-#define BitScanForwardPtr BitScanForward64
-#else
-#define BitScanForwardPtr BitScanForward
-#endif
+#if defined(_MSC_VER)
+
+// Define wrappers over the non-underscore versions of the BitScan* APIs. The PAL defines these already.
+// We've #undef'ed the definitions in winnt.h for these names to avoid confusion.
+
+inline BOOLEAN BitScanForward(DWORD* Index, DWORD Mask)
+{
+    return ::_BitScanForward(Index, Mask);
+}
+
+inline BOOLEAN BitScanReverse(DWORD* Index, DWORD Mask)
+{
+    return ::_BitScanReverse(Index, Mask);
+}
+
+#if defined(HOST_64BIT)
+inline BOOLEAN BitScanForward64(DWORD* Index, DWORD64 Mask)
+{
+    return ::_BitScanForward64(Index, Mask);
+}
+
+inline BOOLEAN BitScanReverse64(DWORD* Index, DWORD64 Mask)
+{
+    return ::_BitScanReverse64(Index, Mask);
+}
+#endif // defined(HOST_64BIT)
+
+#endif // _MSC_VER
 
 template <typename T, int size>
 inline constexpr unsigned ArrLen(T (&)[size])