raspberrypi / pico-sdk

BSD 3-Clause "New" or "Revised" License
3.25k stars 838 forks source link

shifting int64_t and uint64_t returns incorrect result #1689

Closed ajo-em closed 2 months ago

ajo-em commented 2 months ago

Error when left shifting uint64_t or int64_t

uint64_t ui64 = 0x01; int64_t i64 = 0x01; uint64_t uResult; int64_t sResult;

uResult = ui63 << 30; // returns 0x0000 0000 4000 000 --> this is OK ! uResult = ui63 << 31; // returns 0xFFFF FFFF 8000 0000 --> this is not OK ! uResult = ui63 << 32; // returns 0x000 0000 0000 0000 --> this is not OK !

sResult = i64 << 30; // returns 0x0000 0000 4000 000 --> this is OK ! sResult = i64 << 31; // returns 0xFFFF FFFF 8000 0000 --> this is not OK ! sResult = i64 << 32; // returns 0x000 0000 0000 0000 --> this is not OK !

Unexpected result when right shifting int64_t

As I read there is no exact definition for right shifting of signed intergers. There are implementations where MSB is copied when shifting right. (Which I would prefer !) It's also OK if zero is shifted in from left side.

I would expect that there is the same result, if the shift width is a constant number or if it a variable. My experiment was to shift by 1 in a loop and also shift by a width defined by variable. My expectation was, that there is the same result. But it is Not !

int64_t i64 = 0x8000 0000 0000 0000; int16_t n = 1;

sResult = i64 >> 1; // returns 0xC000 0000 0000 0000
sResult = i64 >> n; // returns 0x4000 0000 0000 0000

 single shift copies MSB;
 shift by n shifts n zeroes from the left side  ( n = 1 .. 63)

right shifting int32_t has the same effect:
shifting by n shifts in zeroes, while shifting by 1 shifts in MSB

Here is some code I used for testing:

/ ============================================================================

void print_i64(int64_t i64)
{
   int32_t  low32 = (int32_t) (0x00000000FFFFFFFF & i64);
   int32_t high32 = (int32_t) (0x00000000FFFFFFFF & ((uint64_t)i64 >> 32));
   printf("0x%08x%08x",high32,low32);
}

void print_ui64(uint64_t ui64)
{
   int32_t  low32 = (int32_t) (0x00000000FFFFFFFF & ui64);
   int32_t high32 = (int32_t) (0x00000000FFFFFFFF & (ui64 >> 32));
   printf("0x%08x%08x",high32,low32);
}

// ============================================================================

// ==================================================================
// shift left unsigned 64
void do_ShiftLeft_ui64(void)
{
  printf("Test bit shift left - 1 to 64 - unsigned64 \n");
  printf("========================================== \n");

  uint64_t ui64_arr[65];
  uint64_t sh1_Val = 0x01;
  uint32_t i;
  for ( i = 1; i < 64; ++i)
  {
    // prod1_sr[i] = prod1 >> i;
    sh1_Val = sh1_Val << 1;
    printf("shift_l_width = %d \n",i);

    printf("single shift-l value          : ");
    print_ui64( sh1_Val);
    printf("\n");

    ui64_arr[i] = 0x01 << i;
    printf("multi bit-shift value,  n = %2d: ",i);
    print_ui64( ui64_arr[i]);
    if(sh1_Val != ui64_arr[i]) printf("  <------");
    printf("\n\n");
  }
  sleep_ms(100);
}
// ==================================================================

// ==================================================================
// shift left signed 64
void do_ShiftLeft_i64(void)
{
  printf("Test bit shift left - 1 to 64 - signed64 \n");
  printf("======================================== \n");

  int64_t i64_arr[65];
  int64_t sh1_Val = 0x01;
  uint32_t i;

  for ( i = 1; i < 64; ++i)
  {
    // prod1_sr[i] = prod1 >> i;
    sh1_Val = sh1_Val << 1;
    printf("shift_l_width = %d \n",i);

    printf("single shift-l value          : ");
    print_i64( sh1_Val);
    printf("\n");

    i64_arr[i] = 0x01 << i;
    printf("multi bit-shift value,  n = %2d: ",i);
    print_i64( i64_arr[i]);
    if(sh1_Val != i64_arr[i]) printf("  <------");
    printf("\n\n");
  }
  sleep_ms(100);
}
// ==================================================================

// ==================================================================
// shift right signed 32
void do_ShiftRight_i32(void)
{
  printf("Test bit shift right - 1 to 32 - signed32 \n");
  printf("========================================= \n");

  int32_t i32_arr[33];
  int32_t shr_Val = 0x80000000;
  uint32_t i;
  for ( i = 1; i < 33; ++i)
  {
    printf("shift_r_width = %d \n",i);

    shr_Val = shr_Val >> 1;
    printf("single shift-r value          : 0x%08x \n", shr_Val);

    i32_arr[i] = 0x80000000 >> i;
    printf("multi bit-shift value,  n = %2d: 0x%08x", i, i32_arr[i]);
    if(shr_Val != i32_arr[i]) printf("  <------");
    printf("\n\n");

  }
  sleep_ms(100);
}
// ==================================================================

(small edit by lurch to put code-block in backticks)

peterharperuk commented 2 months ago

Curious, I don't see that behaviour...

test start unsigned left shift ui64=40000000 ui64=80000000 ui64=100000000 signed left shift i64=40000000 i64=80000000 i64=100000000 signed right shift i64=c000000000000000 i64=c000000000000000 test end

kilograham commented 2 months ago

sResult = i64 << 31; // returns 0xFFFF FFFF 8000 0000 --> this is not OK !

This seems like the shifted 32 bits are being sign extended to 64 bits, which your code shouldn't/doesn't do; what compiler are you using, are you using debug... it seems like an unlikely bug even in an ancient GCC compiler tho.

ajo-em commented 2 months ago

Thanks for your help !!

My GCC-version is 13.2.1 20231009 ( not so very ancient )

I used it in Debug-Mode. But there was the same result with Release-Mode.

Meanwhile I could find the cause of the seen behavior ! Your comments lead me to the suspicion that there were data types used and eventually were internally converted which I did not expect.

These changes helped :

i32_arr[i] = 0x80000000 >> i; -----> i32_arr[i] = (int32_t)0x80000000 >> i;

ui64_arr[i] = 0x01 << i; -----> ui64_arr[i] = (uint64_t)0x01 << i;

i64_arr[i] = 0x01 << i; -----> i64_arr[i] = (int64_t)0x01 << i;

i64_arr[i] = 0x8000000000000000 >> i; -----> i64_arr[i] = (int64_t)0x8000000000000000 >> i;

warcow105 commented 2 months ago

Yeah, need to be careful with that. From cppreference "The type of the integer literal is the first type in which the value can fit...". Extra bits are not added when you shift past the bit size of the type. So you should specify your literal as the type you actually want as a result of the operation.

peterharperuk commented 2 months ago

Closing this. Please reopen if you disagree.