[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
 
+  // Returns true if the instruction I can be hoisted to the preheader of FC0.
+  // SafeToHoist contains the instructions that are known to be safe to hoist.
----------------
please add `\p` before parameters. e.g., `instruction \p I`.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1042
 
+  // Returns true if the instruction I can be hoisted to the preheader of FC0.
+  // SafeToHoist contains the instructions that are known to be safe to hoist.
----------------
Whitney wrote:
> please add `\p` before parameters. e.g., `instruction \p I`.
You may want to specific is the end of the preheader.


================
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;
----------------
Can combine `!I.mayReadFromMemory() && !I.mayWriteToMemory()` to `!I.mayReadOrWriteMemory()`.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1103
+
+  // Returns true if the instruction I can be sunk to the exit block of FC1.
+  bool canSinkInst(Instruction &I, const FusionCandidate &FC1) const {
----------------
You may want to specify is to the beginning of the exit block.


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

Not sure what you mean by that. I can see that you changed it to traverse memory reads and writes, LGTM.


================
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()) {
----------------
aaronkintel wrote:
> Whitney wrote:
> > 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.
> sUnordered() calls isVolatile() and checks atomicity under the hood.
Right, but `isUnordered()` is not a member function of LLVM::Instruction. Your new change for this comment LGTM.


================
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
----------------
aaronkintel wrote:
> Whitney wrote:
> > How is this change related to this patch?
> Lit test was otherwise broken. 
Can you please elaborate more on why it was broken?


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

https://reviews.llvm.org/D131606