[PATCH] D131606: [Loop Fusion] Sink/hoist memory instructions between loop fusion candidates

[Whitney Tsang via Phabricator via llvm-commits]

Whitney added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1042
 
+  bool canHoistInst(Instruction &I,
+                    const SmallVector<Instruction *, 4> &SafeToHoist,
----------------
Please add a description of this function and the expected result/meaning of the arguments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1046
+                    const FusionCandidate &FC0) const {
+    // First check if can be hoisted
+    // If the operands of this instruction dominate the FC0 Preheader
----------------
This comment no longer needed in this context, as this function is for checking if the instruction can be hoisted.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1065
+    // If this isn't a memory inst, hoisting is safe
+    if (!I.mayReadFromMemory() && !I.mayWriteToMemory()) {
+      return true;
----------------
No need braces for single instruction block.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1082
+
+    for (Instruction &HeaderInst : *FC0.Header) {
+      if (auto D = DI.depends(&I, &HeaderInst, true)) {
----------------
Why only considering instructions in the header and not any other blocks in FC0 loop?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1097
+
+  bool canSinkInst(Instruction &I, const FusionCandidate &FC1) const {
+    for (User *U : I.users()) {
----------------
Please add a description of this function.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1111
+    // If this isn't a memory inst, sinking is safe
+    if (!I.mayReadFromMemory() && !I.mayWriteToMemory()) {
+      return true;
----------------
Please remove braces.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1115
+
+    for (Instruction &HeaderInst : *FC1.Header) {
+      if (auto D = DI.depends(&I, &HeaderInst, true)) {
----------------
Why only considering instructions in the header and not any other blocks in FC1 loop?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1137
+    // Save the instructions that are not being hoisted, so we know not to hoist
+    // mem insts that they dominate
+    SmallVector<Instruction *, 4> NotHoisting;
----------------
nit: add a period at the end of a sentence. 


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1155
 
-      // First check if can be hoisted
-      // If the operands of this instruction dominate the FC0 Preheader
-      // target block, then it is safe to move them to the end of the FC0
-      const BasicBlock *FC0PreheaderTarget =
-          FC0.Preheader->getSingleSuccessor();
-      assert(FC0PreheaderTarget &&
-             "Expected single successor for loop preheader.");
-      bool CanHoistInst = true;
-      for (Use &Op : I.operands()) {
-        if (auto *OpInst = dyn_cast<Instruction>(Op)) {
-          bool OpHoisted = is_contained(SafeToHoist, OpInst);
-          // Check if we have already decided to hoist this operand. In this
-          // case, it does not dominate FC0 *yet*, but will after we hoist it.
-          if (!(OpHoisted || DT.dominates(OpInst, FC0PreheaderTarget))) {
-            CanHoistInst = false;
-            break;
-          }
+      if (auto SI = dyn_cast<StoreInst>(&I)) {
+        if (!SI->isUnordered()) {
----------------
Should we also skip other non-store instructions if they are volatile or atomic?

I can see that LLVM::Instruction has function `isVolatile()` and `isAtomic()` to check those.


================
Comment at: llvm/test/Transforms/LoopFusion/no_sink_hoist_load.ll:1
+; RUN: opt -S -loop-simplify -loop-fusion -debug-only=loop-fusion < %s 2>&1 | FileCheck %s
+; REQUIRES: asserts
----------------
Do we have test case for volatile or atomic?


================
Comment at: llvm/test/Transforms/LoopFusion/no_sink_hoist_unknown_function.ll:13
 
 ; CHECK:body1: 
 body1:  ; preds = %pre1, %body1
----------------
`; CHECK-NOT: call void @unknown_func()`?


================
Comment at: llvm/test/Transforms/LoopFusion/no_sink_hoist_unknown_function.ll:26
 
 ; CHECK: body2:
 body2:  ; preds = %pre2, %body2
----------------
`; CHECK-NOT: call void @unknown_func()`?


================
Comment at: llvm/test/Transforms/LoopFusion/simple.ll:512
 ; CHECK-NEXT:    store i32 2, i32* [[AJ]], align 4
-; CHECK-NEXT:    [[INC_J]] = add nsw i64 [[J]], 1
+; CHECK-NEXT:    [[INC_J]] = add nsw i64 [[J]], [[ADD]]
 ; CHECK-NEXT:    [[CMP_J:%.*]] = icmp slt i64 [[INC_J]], 100
----------------
How is this change related to this patch?


================
Comment at: llvm/test/Transforms/LoopFusion/sink_load.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -S -loop-simplify -loop-fusion -debug-only=loop-fusion < %s 2>&1 | FileCheck %s
----------------
do we have test cases for hoist memory instructions?


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

https://reviews.llvm.org/D131606