[PATCH] D116469: [AMDGPU][KnownBits] Correct the known bits calculation for MUL_I24.

Sun Jan 2 13:55:05 PST 2022

craig.topper added a comment.

In D116469#3216166 <https://reviews.llvm.org/D116469#3216166>, @foad wrote:

> Both changes look good to me but it's not clear how they are related, and it seems odd to include the generic change in a patch which claims to be AMDGPU-specific. Would it makes sense to commit the generic change separately first?

I'll split them. I think the generic change affect AMDGPU behavior becuase the return value of 0 that is being changed only happens when no bits are known. In that case the isNegative/isNonNegative/isStrictlyPositive would all return false so no calls to setHighBits would occur.

>> As an example of an incorrect result let's say that countMinSignBits
>> returned 1 for the left hand side and 24 for the right hand side.
>> LHSValBits would be 23 and RHSValBits would be 0 and the sum would
>> be 23. This would cause the code to set 9 high bits as zero/one. Now
>> suppose the real values for the left side is 0x8000000 and the right
>> hand side is 0xffffff. The product is 0x00800000 which has 8 sign bits
>> not 9.
>
> "... the real value for the left side is 0x800000 ..."? Your value 0x8000000 has too many zeros.

Fixed. I guess my brain really wanted to write a 32-bit number.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116469