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

[Graham Hunter via Phabricator via llvm-commits]

huntergr added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2707-2708
+    if (!(VOp->hasOneUse() && VOp->hasAllowReassoc())) {
+      candidates = 0U;
+      break;
+    }
----------------
Can just 'return false;' here instead of taking the extra step before returning.

Same with the other checks in this loop.


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:2732
+    if ((L.isLoopInvariant(Ops[0])) || (L.isLoopInvariant(Ops[1])))
+      candidates++;
+  }
----------------
I don't think we need a count here; just a boolean 'CandidateFound' would suffice.


================
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");
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152281