[PATCH] D55851: Implement basic loop fusion pass

[Kit Barton via Phabricator via llvm-commits]

kbarton marked 31 inline comments as done and 2 inline comments as done.
kbarton added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:106-109
+static cl::opt<bool>
+    VerboseFusionDebugging("loop-fusion-verbose-debug",
+                           cl::desc("Enable verbose debugging for Loop Fusion"),
+                           cl::Hidden, cl::init(false), cl::ZeroOrMore);
----------------
Meinersbur wrote:
> [serious] LLVM has an infrastructure for enabling debugging output: `-debug-only`. We should not introduce additional ones.
> 
> At least this option should only be available in assert-builds.
I use this to have two different "levels" of debugging. The debugging available with -debug-only is not as verbose.
If you feel strongly about it, I can remove this and make all debug output equivalent.
In the meantime, I've guarded this use with NDEBUG. 


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:977-980
+    TreeUpdates.push_back(
+        {DominatorTree::Delete, FC0.ExitingBlocks, FC1.Preheader});
+    TreeUpdates.push_back(
+        {DominatorTree::Insert, FC0.ExitingBlocks, FC1.Header});
----------------
Meinersbur wrote:
> [style] Use `emplace_back`?
There is no emplace_back for TreeUpdates (unless I misunderstood your suggestion). 


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1059-1063
+    assert(!verifyFunction(*FC0.Header->getParent(), &errs()));
+    assert(DT.verify());
+    assert(PDT.verify());
+    LI.verify(DT);
+    SE.verify();
----------------
Meinersbur wrote:
> [suggestion] This looks expensive to do. However, the pass manager will do these verifications anyway between passes (if enabled), so it shouldn't be necessary to do here.
I've kept this for now, as this isn't in the pass manager yet. However, I've guarded it under NDEBUG. 
Are you OK with that?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1133
+
+  return PreservedAnalyses::all();
+}
----------------
Meinersbur wrote:
> [serious] If changes are made, the pass should not indicate that it preserves all analyses (including the ones it doesn't know about).
I agree. The pass should not make changes and return true. 
I don't believe this is happening now.


================
Comment at: llvm/test/Transforms/LoopFusion/cannot_fuse.ll:1
+; RUN: opt -S -loop-fusion -debug-only=loop-fusion < %s 2>&1 | FileCheck %s
+
----------------
Meinersbur wrote:
> [serious] If you are testing for `llvm::dbgs()` output, you need to add a `REQUIRES: asserts` line. 
> 
> Another problem is that `2>&1` this mixes stdout and stderr output. stdout is buffered, stderr is not. How these are merged by `2>&1` is undefined. You can switch off stdout by using `opt -disable-output`.
> 
> If possible, using `opt -analyze` is a better way to verify pass-specific output because adding another `LLVM_DEBUG` during debugging sessions can lead to test case failures. However, checking `-debug` output is common in the llvm tests.
I don't see an (easy) way to use analyze to get this information.
For now, I've added the REQUIRES: asserts. 
I'll think if there is a way to use analyze in the future to accomplish this. 


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

https://reviews.llvm.org/D55851