[PATCH] D152281: [Transforms][LICM] Add the ability to undo unprofitable reassociation

[Paul Osmialowski via Phabricator via llvm-commits]

pawosm01 marked 3 inline comments as done.
pawosm01 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2707-2708
+    if (!(VOp->hasOneUse() && VOp->hasAllowReassoc())) {
+      candidates = 0U;
+      break;
+    }
----------------
huntergr wrote:
> Can just 'return false;' here instead of taking the extra step before returning.
> 
> Same with the other checks in this loop.
Yes, same in the other places.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2732
+    if ((L.isLoopInvariant(Ops[0])) || (L.isLoopInvariant(Ops[1])))
+      candidates++;
+  }
----------------
huntergr wrote:
> I don't think we need a count here; just a boolean 'CandidateFound' would suffice.
It isn't as easy as it seems. As it turns, this transformation shouldn't be applied for every situation where at least one candidate is found.

There are two problems here:

1. a simple expression like inv1 * var1 * inv2 will be found as a candidate and transformed into var1 * (inv1 * inv2) where (inv1 * inv2) part will be hoisted. But this expression itself can be a part of a longer expression, e.g. inv1 * var1 * inv2 + inv1 * var2 * inv2 + inv1 * var3 * inv2 which before the introduction of my change could easily be transformed into (var1 + var2 + var3) * (inv1 * inv2) with (inv1 * inv2) hoisted. The transformation I'm proposing should rather focus on the expressions described in the comment block placed before this function which usually  require undoing the actions of some previous pass (namely, the Reasociate pass), and leave simpler cases alone and make the existing test cases supporting them just pass without any awfully messy change. Hence, I've added `AnyAdds` variable to make sure the intended expression patterns are being found. The previous oversimlified condition eliminated such cases by a sheer coincidence.

2. with the expression long enough and small enough trip count there is a possibility that the transformation leading to the hoisting can cause performance degradation. If most of the operations in a particularly long expression involves non-invariants, there will be more multiplications introduced than operations hoisted. By changing the type of the `Candidates` counter to a signed integer and decrementing it whenever there is no invariant in the operation (hence no candidate for hoisting), the applicability of this transformation can be limited to the situations where the numbers of non-invariant operations and hoisting candidates are at least equal. A crude estimate and naïve one, I can admit that, but anything better would overcomplicate the logic here, e.g. by introducing instruction cost estimation and trip count estimation, the cost of doing it can easily outgrow any benefits.



================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2744-2754
+    for (int i = 0; i < 2; i++) {
+      if (i || L.isLoopInvariant(Ops[i])) {
+        assert(!Factored);
+        IRBuilder<> Builder(L.isLoopInvariant(Ops[i])
+                                ? (candidates--, Preheader->getTerminator())
+                                : Op);
+        Factored = Builder.CreateFMulFMF(Ops[i], Factor, &I, "factor.op.fmul");
----------------
huntergr wrote:
> Using a loop over the ops with a break after the substitution is performed feels a bit unwieldy. Could you try recording the Ops in a known order in Changes (probably using std::swap as you did before), so you know that e.g. the first one is always loop invariant? I don't think we need to maintain the order of operations for an fmul so you could overwrite both, but if there is a good reason to maintain the order then you could just make the nested pair in changes a tuple instead and record the index of the invariant operand.
yeah, I've simplified this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152281