[PATCH] D55851: Implement basic loop fusion pass

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Great stuff. This looks like it will be very useful. I have a few thoughts/questions/nits.



================
Comment at: llvm/include/llvm/Transforms/Scalar/LoopFuse.h:26
+public:
+   PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM);
+};
----------------
Two spaces.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:612
+      UncomputableTripCount++;
+      LLVM_DEBUG(dbgs() << "Trip count of first loop could not be computed!");
+      return false;
----------------
second loop


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:719
+
+          LLVM_DEBUG(dbgs()
+                     << "Candidate Set (after inserting TargetCandidate):\n"
----------------
This debug message is (essentially) repeated?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:747
+      if (OldL.contains(ExprL)) {
+        //auto *Step = Expr->getStepRecurrence(SE);
+        bool Pos = SE.isKnownPositive(Expr->getStepRecurrence(SE));
----------------
Commented code


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:858
+
+      // TODO: Can we actually use the dependence info analysis here?
+      return false;
----------------
I would imagine, although I'm not sure, that there would at least be a lot of bugs here. We are dealing with different loops, but we can say that they are very similar.

What does it currently give? Anything useful? The SCEV checks you are doing above is obviously quite similar to what DA would be trying to do, but with the added loop rewrite. It would be a shame to duplicate all the effort but may, I guess, be necessary if DA doesn't do well enough.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:966
+    // values might not dominate the exiting branch. While we do not generally
+    // thest if this is the case but simply insert intermediate phi nodes, we
+    // need to make sure these intermediate phi nodes have different
----------------
thest -> test?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:984
+
+    // Theg old exiting block of the first loop (FC0) has to jump to the header
+    // of the second as we need to execute the code in the second header block
----------------
Theg


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1003
+                                                          FC1.Header);
+    TreeUpdates.push_back(
+        {DominatorTree::Delete, FC0.ExitingBlocks, FC1.Preheader});
----------------
It can sometimes be better to insert edges into the DT before deleting old ones. It keep the tree more intact (especially pdts), with less subtrees becoming unreachable and less recalculations needed. It means it can be simpler and quicker for the updates.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1015
+    // Moves the phi nodes from the second to the first loops header block.
+    while (PHINode *PHI = dyn_cast<PHINode>(&FC1.Header->front())) {
+      if (SE.isSCEVable(PHI->getType()))
----------------
Is there anywhere that we check that these won't have phi operands from inside the first loop? i.e. that the phis can be moved into the first loop / before all of it's instructions.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1087
+    assert(!verifyFunction(*FC0.Header->getParent(), &errs()));
+    assert(DT.verify());
+    assert(PDT.verify());
----------------
You may want to add DominatorTree::VerificationLevel::Fast otherwise this might be quite slow.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1118
+
+    AU.addPreserved<ScalarEvolutionWrapperPass>();
+    AU.addPreserved<DominatorTreeWrapperPass>();
----------------
Looks like the code is preserving LI too (I'm not sure if you can/should preserve SE without preserving LI.)


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

https://reviews.llvm.org/D55851