[libcxx-commits] [PATCH] D134625: Summary:bug fix! Rotation direction on `__countl_zero()`probably unnoticed because only affects rare cases(does not affect 128 bit ints because rotation is effectively swap)(does not affect integers of sizes less or equal to 64bits)

[Louis Dionne via Phabricator via libcxx-commits]

ldionne commandeered this revision.
ldionne added a reviewer: Delta-dev-99.
ldionne added a comment.

[Github PR transition cleanup]

I revisited this in the context of cleaning up our review queue. I think the author of the patch is right in theory, however this can't be tested because we don't support types larger than 128 bits (and for these types both before and after the patch are equivalent). But if we did support e.g. 256 bit integers, our current algorithm would give the wrong answer. Here's a walkthrough with a 256 bit integer.

Current algorithm:

  // We have the following 256 bit number. The answer to countl_zero should be 0 because the leftmost bit is set:
  // 10000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
  
  // We start by doing __rotr(__t, unsigned long long digits == 64), which gives us the following number:
  // 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   10000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
  
  // Now countl_zero of that number (casted to unsigned long long) is:
  // countl_zero(00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000) == 64
  
  // Since that is equal to numeric_limits<unsigned long long>::digits, don't get into the if-then-break, we set __ret = 64 and we keep going.
  
  // Now we do __rotr(__t, unsigned long long digits) again, which gives us the following number:
  // 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   10000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
  
  // We countl_zero again, which gives us 64 and we set __ret = 128. We keep going again.
  
  // We do __rotr(__t, unsigned long long digits) again, which gives us the following number:
  // 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   10000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
  
  // This time countl_zero(10000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000) is 0, which is not equal to numeric_limits<unsigned long long>::digits. So we enter the if-then-break and we return 128.

If instead we used `__rotl(__t, unsigned long long digits)` as proposed by this patch, we'd get this:

  // This is the number we start with, same as above:
  // 10000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
  
  // We now do __rotl(__t, unsigned long long digits == 64), which gives us the following number:
  // 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000   10000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
  
  // Now countl_zero of that number (casted to unsigned long long) is:
  // countl_zero(10000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000) == 0
  
  // That is equal to 0, which is not the same as __ulldigits, so we enter the if-then-break and we return __ret which is 0.

I also tried with the number `0x0000000000000000000000000000000080000000000000000000000000000000` as suggested and the same thing happens, we give the wrong answer. When you think about it it really makes sense, we need to "eat" the `0` digits to the left of the first digit that's `1` first, that's the only correct way to break down a large width integer into `unsigned long long` chunks. So I'll commandeer this patch and rebase it to finish it up.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134625/new/

https://reviews.llvm.org/D134625