[PATCH] D55851: Implement basic loop fusion pass

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Initial comments from my side.



================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:142-143
+
+  FusionCandidate(Loop *L, const DominatorTree *DT,
+                  const PostDominatorTree *PDT)
+      : Preheader(L->getLoopPreheader()), Header(L->getHeader()),
----------------
kbarton wrote:
> Meinersbur wrote:
> > [suggestion] IMHO it is good practice to not have much logic in object constructors. Reasons:
> > 1. When exceptions are thrown, the object is partially initialized
> > 2. With class polymorphism, calling virtual functions will call the non-overidden method.
> > 3. It is not possible to return anything, in this case, an error code.
> > 
> > 1. and 2. do not apply to this code. Point 3. is handled using the `Valid` flag.
> > 
> > Nonetheless, I think having lots of logic in constructor is a bad pattern. Instead, I suggest to have something return nullptr or `llvm::ErrorOr`.
> I understand your points here. I did not realize issue #2 (although I agree it doesn't apply here). 
> 
> However, if I understand your suggestion, I would need to add an initialize method (or something similar) to do the logic that is currently in the constructor and return a success/fail code. The problem is that before anything can be done with the object, you need to ensure that it has been initialized. This can be accomplished by adding a boolean flag (isInitialized) to the class and ensuring that all accessors check that before doing anything with it. However, I think that is more complicated then leaving the initialization logic in the constructor.
> 
> That said, I'm not particularly tied to either approach. I can change the implementation if you feel strongly. Or, if there is an alternative that I'm not seeing, I'm happy to do that instead. 
It is not uncommon in LLVM to do it this way. I personally do not try to force a solution similar to `bool createXXX(...)` but I also do not necessarily object if there is an actual reason.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:591-594
+    if (!Predicates.isAlwaysTrue()) {
+      LLVM_DEBUG(dbgs() << "Trip counts are predicated but predication "
+                           "is not yet supported!");
+      PredicatedTripCount++;
----------------
Meinersbur wrote:
> [suggestion] If predication is not supported, why not using `ScalarEvolution::getBackedgeTakenCount()`?
Because, as an example and not the only reason, we could easily generate statistics on the impact predication support might have.


================
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()))
----------------
kbarton wrote:
> dmgreen wrote:
> > 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.
> This is a really good catch!] We completely missed this.
> I've added a check in dependencesAllowFusion that walks through the PHIs in the header of FC1 and check if the incoming block is in FC0 then it is the header block. If it is not, then we do not fuse. 
I don't get this. Could one of you produce a problematic example for me? (also good as a test case)


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:7
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
----------------
We need to update the licence ;)


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:111
+                           cl::Hidden, cl::init(false), cl::ZeroOrMore);
+#endif
+
----------------
You should keep it this way. Other passes do it similarly.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:848
+             !DepResult->getNextSuccessor() &&
+             "TODO: JD: Implement pred/succ dependence handling!");
+
----------------
This should probably be a conditional and a `return false` ;)


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1162
+  PA.preserve<PostDominatorTreeAnalysis>();
+  PA.preserve<ScalarEvolutionAnalysis>();
+  return PA;
----------------
As above, preserve LoopAnalysis.


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

https://reviews.llvm.org/D55851