[llvm-commits] CVS: llvm/include/llvm/Support/MathExtras.h

[Jeff Cohen]

How is it any worse than checking for GCC?

It's treated as an intrinsic when optimizations are enable.  It directly 
generates a bswap instruction.  Same as the conditional code for GCC.

Reid Spencer wrote:
> Jeff,
>
> On Thu, 2007-03-22 at 14:12 -0500, Jeff Cohen wrote:
>   
>> Changes in directory llvm/include/llvm/Support:
>>
>> MathExtras.h updated: 1.43 -> 1.44
>> ---
>> Log message:
>>
>> Be more explicit concerning argument sizes.
>> Use VC++ byteswap intrinsics.
>>     
>
> ... snip ...
>
>   
>> @@ -93,22 +93,30 @@
>>  
>>  /// ByteSwap_16 - This function returns a byte-swapped representation of the
>>  /// 16-bit argument, Value.
>> -inline unsigned short ByteSwap_16(unsigned short Value) {
>> -  unsigned short Hi = Value << 8;
>> -  unsigned short Lo = Value >> 8;
>> +inline uint16_t ByteSwap_16(uint16_t Value) {
>> +#if defined(_MSC_VER) && !defined(_DEBUG)
>> +  // The DLL version of the runtime lacks these functions (bug!?), but in a
>> +  // release build they're replaced with BSWAP instructions anyway.
>> +  return _byteswap_ushort(Value);
>> +#else
>> +  uint16_t Hi = Value << 8;
>> +  uint16_t Lo = Value >> 8;
>>    return Hi | Lo;
>> +#endif
>>  }
>>     
>
> Pedantically, such an ifdef is not allowed to occur in this header file.
> It can only occur in lib/System. LLVM is supposed to be platform/machine
> agnostic everywhere except lib/System. Is there a real advantage to
> using _byteswap_ushort? Seems to me that the function call overhead
> would negate any speed advantage. Is this thing inline? I would prefer
> that we just leave the code as is. If you really feel its necessary to
> do this for release versions, please create an inline function in
> lib/System that works for all platforms.
>
>   
>>  
>>  /// ByteSwap_32 - This function returns a byte-swapped representation of the
>>  /// 32-bit argument, Value.
>> -inline unsigned ByteSwap_32(unsigned Value) {
>> +inline uint32_t ByteSwap_32(uint32_t Value) {
>>     
> The type change is fine, but ..
>   
>>  #if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3)
>>  	return __builtin_bswap32(Value);
>> +#elif defined(_MSC_VER) && !defined(_DEBUG)
>> +  return _byteswap_ulong(Value);
>>     
> I really don't like this unless you put this in lib/System.
>   
>>  #else
>> -  unsigned Byte0 = Value & 0x000000FF;
>> -  unsigned Byte1 = Value & 0x0000FF00;
>> -  unsigned Byte2 = Value & 0x00FF0000;
>> -  unsigned Byte3 = Value & 0xFF000000;
>> +  uint32_t Byte0 = Value & 0x000000FF;
>> +  uint32_t Byte1 = Value & 0x0000FF00;
>> +  uint32_t Byte2 = Value & 0x00FF0000;
>> +  uint32_t Byte3 = Value & 0xFF000000;
>>    return (Byte0 << 24) | (Byte1 << 8) | (Byte2 >> 8) | (Byte3 >> 24);
>>  #endif
>>  }
>> @@ -118,9 +126,11 @@
>>  inline uint64_t ByteSwap_64(uint64_t Value) {
>>  #if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 3)
>>  	return __builtin_bswap64(Value);
>> +#elif defined(_MSC_VER) && !defined(_DEBUG)
>> +  return _byteswap_uint64(Value);
>>     
>
> Same deal.
>
>   
>>  #else
>> -  uint64_t Hi = ByteSwap_32(unsigned(Value));
>> -  uint64_t Lo = ByteSwap_32(unsigned(Value >> 32));
>> +  uint64_t Hi = ByteSwap_32(uint32_t(Value));
>> +  uint32_t Lo = ByteSwap_32(uint32_t(Value >> 32));
>>    return (Hi << 32) | Lo;
>>  #endif
>>  }
>> @@ -129,7 +139,7 @@
>>  /// counting the number of zeros from the most significant bit to the first one
>>  /// bit.  Ex. CountLeadingZeros_32(0x00F000FF) == 8.
>>  /// Returns 32 if the word is zero.
>> -inline unsigned CountLeadingZeros_32(unsigned Value) {
>> +inline unsigned CountLeadingZeros_32(uint32_t Value) {
>>    unsigned Count; // result
>>  #if __GNUC__ >= 4
>>    // PowerPC is defined for __builtin_clz(0)
>> @@ -142,7 +152,7 @@
>>    Count = 0;
>>    // bisecton method for count leading zeros
>>    for (unsigned Shift = 32 >> 1; Shift; Shift >>= 1) {
>>     
>
> Why not uint32_t for Shift?
>
>   
>> -    unsigned Tmp = Value >> Shift;
>> +    uint32_t Tmp = Value >> Shift;
>>      if (Tmp) {
>>        Value = Tmp;
>>      } else {
>> @@ -170,7 +180,7 @@
>>      if (!Value) return 64;
>>      Count = 0;
>>      // bisecton method for count leading zeros
>> -    for (uint64_t Shift = 64 >> 1; Shift; Shift >>= 1) {
>> +    for (unsigned Shift = 64 >> 1; Shift; Shift >>= 1) {
>>     
>
> Why not uint32_t? For conformity with the rest of your changes?
>
>   
>>        uint64_t Tmp = Value >> Shift;
>>        if (Tmp) {
>>          Value = Tmp;
>> @@ -180,7 +190,7 @@
>>      }
>>    } else {
>>      // get hi portion
>> -    unsigned Hi = Hi_32(Value);
>> +    uint32_t Hi = Hi_32(Value);
>>  
>>      // if some bits in hi portion
>>      if (Hi) {
>> @@ -188,7 +198,7 @@
>>          Count = CountLeadingZeros_32(Hi);
>>      } else {
>>          // get lo portion
>> -        unsigned Lo = Lo_32(Value);
>> +        uint32_t Lo = Lo_32(Value);
>>          // same as 32 bit value
>>          Count = CountLeadingZeros_32(Lo)+32;
>>      }
>> @@ -201,7 +211,7 @@
>>  /// counting the number of zeros from the least significant bit to the first one
>>  /// bit.  Ex. CountTrailingZeros_32(0xFF00FF00) == 8.
>>  /// Returns 32 if the word is zero.
>> -inline unsigned CountTrailingZeros_32(unsigned Value) {
>> +inline unsigned CountTrailingZeros_32(uint32_t Value) {
>>  #if __GNUC__ >= 4
>>    return Value ? __builtin_ctz(Value) : 32;
>>  #else
>> @@ -262,7 +272,7 @@
>>  /// Log2_32 - This function returns the floor log base 2 of the specified value, 
>>  /// -1 if the value is zero. (32 bit edition.)
>>  /// Ex. Log2_32(32) == 5, Log2_32(1) == 0, Log2_32(0) == -1, Log2_32(6) == 2
>> -inline unsigned Log2_32(unsigned Value) {
>> +inline unsigned Log2_32(uint32_t Value) {
>>    return 31 - CountLeadingZeros_32(Value);
>>  }
>>  
>> @@ -275,7 +285,7 @@
>>  /// Log2_32_Ceil - This function returns the ceil log base 2 of the specified
>>  /// value, 32 if the value is zero. (32 bit edition).
>>  /// Ex. Log2_32_Ceil(32) == 5, Log2_32_Ceil(1) == 0, Log2_32_Ceil(6) == 3
>> -inline unsigned Log2_32_Ceil(unsigned Value) {
>> +inline unsigned Log2_32_Ceil(uint32_t Value) {
>>    return 32-CountLeadingZeros_32(Value-1);
>>  }
>>     
>
> Reid.
>
>
>
>
>