[PATCH] D71165: [LoopFusion] Move instructions from FC0.Latch to FC1.Latch.

[Whitney Tsang via Phabricator via llvm-commits]

Whitney marked 3 inline comments as done.
Whitney added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1134
+  void moveInsts(BasicBlock &FromBB, BasicBlock &ToBB, DomTreeUpdater &DTU) {
+    for (auto It = ++FromBB.rbegin(); It != FromBB.rend();) {
+      Instruction *MovePos = ToBB.getFirstNonPHIOrDbg();
----------------
Meinersbur wrote:
> [style]
> ```
> for (Instruction &I : drop_begin(reverse(FromBB), 1))
> ```
`It` need to be incremented before `moveBefore` as it move instructions out of FromBB.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1135
+    for (auto It = ++FromBB.rbegin(); It != FromBB.rend();) {
+      Instruction *MovePos = ToBB.getFirstNonPHIOrDbg();
+      Instruction &I = *It;
----------------
Meinersbur wrote:
> [suggestion] Can this be hoisted before the loop?
Is intentionally putted inside, `ToBB.getFirstNonPHIOrDbg()` returns a different value every iteration, as it will be the previous moved instruction.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1139-1142
+      if (isSafeToMoveBefore(I, *MovePos, DT, PDT, DI))
+        I.moveBefore(MovePos);
+      else
+        break;
----------------
Meinersbur wrote:
> [style] https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return
> ```
> if (!isSafeToMoveBefore(I, *MovePos, DT, PDT, DI))
>   break;
> 
> I.moveBefore(MovePos);
> ```
I will actually remove the `else break;` as we want to move as much safe instructions as possible.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71165