[PATCH] D111523: [AMDGPU] Don't emit 24 bit mul intrinsic for > 32 bit result.

[Jay Foad via Phabricator via llvm-commits]

foad added a comment.

In D111523#3056260 <https://reviews.llvm.org/D111523#3056260>, @rampitec wrote:

> In D111523#3056031 <https://reviews.llvm.org/D111523#3056031>, @foad wrote:
>
>> In D111523#3055898 <https://reviews.llvm.org/D111523#3055898>, @rampitec wrote:
>>
>>> There should be nothing wrong with mul24 regardless of the destination type. If a 64 bit mul fits 24 bit it still can use 24 bit mul, just extended to 64 bit.
>>
>> No, I think this patch makes sense. We were only checking that the inputs fit in 24 bits. A full 24-bit multiply would have a 48 bit result, which you could safely extend to 64 bits. But mul24 gives you a truncated 32-bit result, which you can't safely extend to 64 bits.
>
> Should we check `numBits(LHS) + numBits(RHS) <= DstTy.sizeInBits() || Size <= 32` instead?

No, that would still go wrong for the original case of multiplying two 24-bit numbers to get a 64-bit result, because the intermediate 48-bit result will get truncated to 32 bits by the mul24 instruction before it gets extended to 64 bits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111523