[PATCH] D65464: [LoopFusion] Add ability to fuse guarded loops

[Kit Barton via Phabricator via llvm-commits]

kbarton marked an inline comment as done.
kbarton added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:166
+        Latch(L->getLoopLatch()), L(L), Valid(true), GuardBranch(nullptr),
+        DT(DT), PDT(PDT), ORE(ORE) {
+
----------------
etiotto wrote:
> I think it would be better to pass a const reference to the DominatorTree and PostDominatorTree given that they should never be nullptr ?
I think that should be fine. However, it isn't related to this patch, so I'd prefer to change this in a separate patch. 


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:172
+    if (isRotated())
+      GuardBranch = L->getLoopGuardBranch();
 
----------------
etiotto wrote:
> It would be better to initialize GuardBranch in the init. list: GuardBranch(L->getLoopGuardBranch()) and have getLoopGuardBranch() return nullptr if the loop guard cannot be identified for whatever reason.
getLoopGuardBranch only works on rotated loops (it will assert if called on a non-rotated loop). 


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:251
+  BasicBlock *getNonLoopBlock() const {
+    assert(GuardBranch && "Only valid on guarded loops.");
+    return (GuardBranch->getSuccessor(0) == Preheader)
----------------
etiotto wrote:
> assert GuarbBranch->isConditional()
This is guaranteed by the getGuardBranch routine already.
I can add it here too, though, to make sure that doesn't change over time and break assumptions here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65464