[PATCH] D144045: [InstCombine] avoid sinking fdiv into a loop

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added a comment.

Hi @david-arm, I can see the value of adding LoopInfo and using that to avoid reversing some of LICM's decisions, but I believe this patch makes the decision the wrong way around.
Rather than making the combine unless it can prove that it doesn't reverse LICM, I'd rather see InstCombine not make the combine unless it can prove that it's not reversing LICM.

To clarify, at the moment InstCombine has the following behaviour:

  (A) Combine the reciprocal + fmul => fdiv always

With this patch, the behaviour changes when LoopInfo is available:

  (B) Combine the reciprocal + fmul => fdiv only if the reciprocal has not been hoisted (requires LoopInfo)

We could add a new, more conservative mode where InstCombine does the following:

  (C) Combine the reciprocal + fmul => fdiv only if these live in the same basic block.

Then we can have the following behaviours:

1. Default:
  - If LoopInfo is not available: (C)
  - If LoopInfo is cached: (B)
  - With this new default, it's no longer possible for InstCombine and LICM to conflict, because InstCombine is by default more conservative. This means that having forgotten to enable LoopInfo (when another instance of InstCombine is added to the pipeline) can't lead to regressions for this case.
2. Explicit option to force canonical form regardless of LoopInfo: (A)
  - This way it's still possible to run InstCombine in the mode it currently runs.
  - InstCombine can be forced to run in this mode earlier on in the pipeline before LICM has run.
3. Explicit option to force availability of LoopInfo (i.e. this patch as it is now): (B)
  - I'm not sure how useful this option is in practice, but at least it allows InstCombine to always make the most informed decision, regardless of LoopInfo being intact from a previous pass.

What do you think?



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:629-635
+      auto ShouldSink = [&]() {
+        if (LI) {
+          if (Loop *L = LI->getLoopFor(I.getParent()))
+            return !L->isLoopInvariant(FDiv);
+        }
+        return true;
+      };
----------------
nit: there is little value in creating a lambda and using it only once?


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

https://reviews.llvm.org/D144045