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

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

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?

> 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.

I can try to come up with an AMDGPU test case but not until Tuesday (GMT) at the earliest.

Incidentally there is some very similar logic for 24-bit multiplies in AMDGPUCodeGenPrepare::replaceMulWithMul24.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116469