[PATCH] D154678: [InstCombine] Fold IEEE `fmul`/`fdiv` by Pow2 to `add`/`sub` of exp

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added a comment.

In D154678#4481083 <https://reviews.llvm.org/D154678#4481083>, @arsenm wrote:

> In D154678#4480988 <https://reviews.llvm.org/D154678#4480988>, @goldstein.w.n wrote:
>
>> In D154678#4480022 <https://reviews.llvm.org/D154678#4480022>, @arsenm wrote:
>>
>>> My current thought is this is a worse canonical form for floating point operations and would be better done in codegen. This is going to interfere with FMA matching, plus computeKnownFPClass is going to give up on the integer ops and cast.
>>
>> Is there a reason we can't handle bitcasts in `computeKnownFPClass` using knownbits from the integer type coming in?
>
> The cases that are recognizable would be better served by turning the bitcasts into floating point operations. Adding in some integer add and sub is quite obscuring and would make that more difficult
>
>> I would agree with you if we were replacing the int-to-fp with a fp-to-int, but there is no float except for the constant until the `fdiv`/`fmul`.
>> is `fmul`/`fdiv` by constant really preferable to `add`/`sub` of a constant?
>
> Downstream users of the fmul / fdiv can understand the original float operation but the new integer thing is hopeless. If the original fmul had an fadd user, we could no longer match FMA without a lot more effort. We do have to undo some canonicalizations already for FMA formation,
> but they're simple cases (like fmul x, 2 -> fadd x, x)
>
>>> This is also similar to a codegen combine I’ve been planning on doing to fold fmul by power of 2 to ldexp. That has the same FMA interference properties, but is easier to deal with. Have you considered introducing ldexp instead and breaking that in codegen if ldexp isn’t legal?
>>
>> That does seem more principled. Then we get to remove all the casts.
>>
>> is `(fmul/fdiv C, (uitofp Pow2))` directly equiv to `(ldexp C, -/+ Log2(Pow2))`? (negative log2(pow2) if fdiv)
>
> Yes. I'm pretty sure all the edge cases all work out the same

Okay, am going to abandon this patch and replace with transform to `ldexp`.

Will add `ldexp` -> shift/sub in backend.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154678