[PATCH] D143631: [LTO] Don't let InstCombine re-sink the vastly more expensive fdiv

Tue Feb 14 13:23:59 PST 2023

spatel added a comment.

In D143631#4126961 <https://reviews.llvm.org/D143631#4126961>, @craig.topper wrote:

> In D143631#4126901 <https://reviews.llvm.org/D143631#4126901>, @spatel wrote:
>
>> In D143631#4126874 <https://reviews.llvm.org/D143631#4126874>, @craig.topper wrote:
>>
>>> Do you have a proposed refinement for D87479 <https://reviews.llvm.org/D87479>? Turns out `LI` is only used by InstCombine if its available already in the pipeline so I don't know if it's safe to use that instead of the basic block check.
>>
>> Yes, using LoopInfo was what I was thinking. I just made a rough draft of that, and it handles the motivating case here, and I think it would work on the other examples we've raised too. I can clean it up, test some more, and post it.
>
> I don't think it it will work on the case I raised since IR LICM can't fix it so the damage will happen the first time InstCombine runs without LoopInfo. But I hope I'm wrong.

You're correct - it seems that LICM isn't able to do this in general:

  loop:
    m1 = loopInvariant1 * x;
    m2 = loopInvariant2 * m1;
  -->
  loopInvariantMul = loopInvariant1 * loopInvariant2;
  loop:
    m = x * loopInvariantMul;

I don't know much about LICM - should it be able to do that transform? Or is some other pass responsible for the math reassociation?
This doesn't seem to be fdiv-specific. Here's an example with fadd/fmul; there should only be 1 invariant fadd in the loop:
https://godbolt.org/z/xrs9xfGrf

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143631