[PATCH] D55851: Implement basic loop fusion pass
Michael Kruse via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Dec 18 22:06:45 PST 2018
Meinersbur added a comment.
Thanks for bringing us loop fusion! Here are some review comments, but I did not check the functionality yet.
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:99
+ cl::values(clEnumValN(FUSION_DEPENDENCE_ANALYSIS_SCEV, "scev",
+ "Use the dependence analysis interface"),
+ clEnumValN(FUSION_DEPENDENCE_ANALYSIS_DA, "da",
----------------
Is this the right description for "-loop-fusion-dependence-analysis=scev"? It is the same as for the `da` option.
================
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);
----------------
[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.
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:123-140
+ // Cache of parts of the loop used throughout loop fusion. These should not
+ // need to change throughout the analysis and transformation.
+ BasicBlock *Preheader;
+ BasicBlock *Header;
+ BasicBlock *ExitingBlocks;
+ BasicBlock *ExitBlock;
+ BasicBlock *Latch;
----------------
[suggestion] Add doxygen comments for these fields?
================
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()),
----------------
[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`.
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:154
+ if (BB->hasAddressTaken()) {
+ invalidate();
+ return;
----------------
[remark] In contrast to the other invalidate reason, this one does not have a statistic counter. Intentional?
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:204
+
+ void dump(raw_ostream &OS) const {
+ OS << "\tPreheader: " << (Preheader ? Preheader->getName() : "nullptr")
----------------
[style] Use `LLVM_DUMP_METHOD` pattern for `dump()` methods (See `compiler.h`)
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:271
+namespace {
+using LoopSet = SmallVector<Loop *, 4>;
+
----------------
[remark] Loop**Set** is a vector?
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:276
+// dominates FC1 and FC1 post-dominates FC0.
+using FusionCandidateSet = std::set<FusionCandidate, FusionCandidateCompare>;
+using FusionCandidateCollection = SmallVector<FusionCandidateSet, 4>;
----------------
[remark] Is there a reason to use `std::set`? Usually, the LLVM hash-based sets (`DenseSet`, `SmallPtrSet`, etc) are more performant.
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:282-284
+ for (auto IT = CandSet.begin(); IT != CandSet.end(); ++IT) {
+ OS << *IT << "\n";
+ }
----------------
[style] braces can be removed
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:305-307
+/// outermost level. The descend method is used to go to the next nesting
+/// level. The removeLoop and isRemovedLoop is used to indicate that a given
+/// loop has been removed from the function, and thus is no longer valid.
----------------
[style] I'd prefer method description as a doxygen comment at the methods.
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:321
+
+ // Descend the tree to the next (inner) nesting level
+ void descend() {
----------------
[style] Use doxygen comment?
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:345-347
+ SmallPtrSet<const Loop *, 8> RemovedLoops;
+ unsigned Depth;
+ LoopsOnLevelTy LoopsOnLevel;
----------------
[style] Descriptions of these would be useful.
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:350-355
+static void printLoopSet(const LoopSet &LS) {
+ dbgs() << "****************************\n";
+ for (auto L : LS)
+ printLoop(*L, dbgs());
+ dbgs() << "****************************\n";
+}
----------------
This is only used inside `LLVM_DEBUG`, so the compiler will emit an unused-function warning when compiling in release mode. Guard with `#ifdef NDEBUG`?
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:426-427
+ assert(PDT.verify());
+ LI.verify(DT);
+ SE.verify();
+
----------------
[suggestion] Guard with `#ifndef NDEBUG` to avoid being called in non-assert builds?
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:440-441
+ /// are control-flow equivalent.
+ bool controlFlowEquivalent(const FusionCandidate &FC0,
+ const FusionCandidate &FC1) const {
+ if (VerboseFusionDebugging)
----------------
[style] Start functions with a verb: `isControlFlowEquivalent`
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:455-458
+ if (PDT.dominates(FC0.Preheader, FC1.Preheader))
+ return true;
+ else
+ return false;
----------------
[style] `return PDT.dominates(FC0.Preheader, FC1.Preheader)`
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:494-495
+
+ // JD: We should either do all checks separatly (as partially already done
+ // above), or through the isLoopSimplifyForm() call.
+ if (!FC.L->isLoopSimplifyForm()) {
----------------
[style] This looks like a leftover review comment.
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:517
+ void collectFusionCandidates(const LoopSet &LS) {
+ LLVM_DEBUG(dbgs() << "Entering " << __FUNCTION__ << "\n");
+ for (Loop *L : LS) {
----------------
[Style] Use `LLVM_PRETTY_FUNCTION` instead of `__FUNCTION__`.
Maybe remove entirely? Nowhere I have seen function tracing in LLVM code.
================
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++;
----------------
[suggestion] If predication is not supported, why not using `ScalarEvolution::getBackedgeTakenCount()`?
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:711
+
+ // Rewrite all additive recurrences in a SCEV to use a new loop.
+ class AddRecLoopReplacer : public SCEVRewriteVisitor<AddRecLoopReplacer> {
----------------
[style] Use doxygen comment here?
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:728
+ if (OldL.contains(ExprL)) {
+ auto *Step = Expr->getStepRecurrence(SE);
+ bool Pos = SE.isKnownPositive(Step);
----------------
[style] No reason to use `auto` here
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:745-746
+ private:
+ bool Valid, UseMax;
+ const Loop &OldL, &NewL;
+ };
----------------
[suggestion] Descriptions of these would be useful
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:749-750
+
+ /// Return false if the access functions of @p I0 and @p I1 could cause
+ /// a negative dependence.
+ bool accessDiffIsPositive(const Loop &L0, const Loop &L1, Instruction &I0,
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments | Use `\p` to refer to parameters ]]
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:809-811
+ case FUSION_DEPENDENCE_ANALYSIS_SCEV: {
+ return accessDiffIsPositive(*FC0.L, *FC1.L, I0, I1, AnyDep);
+ }
----------------
[style] The braces seem unnecessary
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:893
+ /// them adjacent.
+ bool adjacent(const FusionCandidate &FC0, const FusionCandidate &FC1) const {
+ return FC0.ExitBlock == FC1.Preheader;
----------------
[style] [[ https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly | Start method names with a verb]]: `isAdjacent`?
================
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});
----------------
[style] Use `emplace_back`?
================
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();
----------------
[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.
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1073-1083
+ FusionCandidateCollection FusionCandidates;
+
+ LoopDepthTree LDT;
+ DomTreeUpdater DTU;
+
+ LoopInfo &LI;
+ DominatorTree &DT;
----------------
[style] I think it is more common in the LLVM case base to have private fields declared at the beginning of the class.
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1094
+
+ void getAnalysisUsage(AnalysisUsage &AU) const override {
+ AU.addRequiredID(LoopSimplifyID);
----------------
[serious] I think LoopFuser is preserving some passes such as ScalarEvolution?
================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1133
+
+ return PreservedAnalyses::all();
+}
----------------
[serious] If changes are made, the pass should not indicate that it preserves all analyses (including the ones it doesn't know about).
================
Comment at: llvm/test/Transforms/LoopFusion/cannot_fuse.ll:1
+; RUN: opt -S -loop-fusion -debug-only=loop-fusion < %s 2>&1 | FileCheck %s
+
----------------
[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.
================
Comment at: llvm/test/Transforms/LoopFusion/cannot_fuse.ll:74-76
+declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
+
+declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
----------------
[remark] I think these can be removed.
================
Comment at: llvm/test/Transforms/LoopFusion/cannot_fuse.ll:269-272
+!3 = !{!4, !4, i64 0}
+!4 = !{!"int", !5, i64 0}
+!5 = !{!"omnipotent char", !6, i64 0}
+!6 = !{!"Simple C/C++ TBAA"}
----------------
[remark] Are these necessary for the test?
================
Comment at: llvm/test/Transforms/LoopFusion/loop_nest.ll:126-140
+declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
+
+declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
+
+
+!llvm.module.flags = !{!0, !1}
+!llvm.ident = !{!2}
----------------
[remark] Can these be removed?
================
Comment at: llvm/test/Transforms/LoopFusion/simple.ll:1
+; RUN: opt -S -loop-fusion < %s 2>&1 | FileCheck %s
+
----------------
[suggestion] Remove `2>&1` (there is no stdout output here)
Repository:
rL LLVM
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55851/new/
https://reviews.llvm.org/D55851
More information about the llvm-commits
mailing list