[PATCH] D139993: [LoopFusion] Sorting of undominated FusionCandidates crashes

[MengXuan Cai via Phabricator via llvm-commits]

Narutoworld requested changes to this revision.
Narutoworld added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:418-420
+    // If two FusionCandidates are in same level of dominator tree then,
+    // they will not be dominates each other. But may be control flow
+    // equivalent. To sort those FusionCandidates nonStrictlyPostDominate is
----------------
Some suggestions about grammer.

If two FusionCandidates are in the same level of dominator tree, they will not dominate each other, but may still be control flow equivalent. To sort shose FusionCandidates, nonStrictlyPostDominate() function is needed.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:427
+    if (bWrongOrder && bRightOrder) {
+      DomTreeNode *LNode = LHS.PDT->getNode(LHSEntryBlock);
+      DomTreeNode *RNode = LHS.PDT->getNode(RHSEntryBlock);
----------------
I think you also need to add comments on this. When "nonStrictlyPostDominate()" is not useful, using the level of PostDominateTree to compare two candidates.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:429-432
+      if (LNode->getLevel() > RNode->getLevel())
+        return true;
+      else
+        return false;
----------------
A more simplified version.

return (LNode->getLevel() > RNode->getLevel() );


================
Comment at: llvm/test/Transforms/LoopFusion/undominated_loops.ll:3
+
+; CHECK: remark: <unknown>:0:0: [function_0]: Loop is not a candidate for fusion: Loop has unknown trip count
+
----------------
I am not sure if it is good to leave remarks inside a testcase. I perfer to not include the remarks since they are not relevant.


================
Comment at: llvm/test/Transforms/LoopFusion/undominated_loops.ll:19
+bb_5:                                         ; preds = %bb_2
+  br i1 undef, label %bb_2, label %bb_7
+
----------------
Maybe try to use **true **or **false **instead. I remembered I recevied a comment before saying limit the use of **undef**


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139993