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

[Aaron K via Phabricator via llvm-commits]

aaronkintel marked 13 inline comments as done.
aaronkintel added inline comments.


================
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;
----------------
bmahjour wrote:
> Whitney wrote:
> > No need braces for single instruction block.
> why the call to mayHaveSideEffects has been removed?
`mayHaveSideEffects()` returns  `mayWriteToMemory() || mayThrow() || !willReturn()`, but we don't want to reject cases that may read to memory anymore.


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


================
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;
----------------
bmahjour wrote:
> Whitney wrote:
> > Please remove braces.
> why the call to mayHaveSideEffects has been removed?
As above.


================
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()) {
----------------
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.


================
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
----------------
Whitney wrote:
> How is this change related to this patch?
Lit test was otherwise broken. 


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

https://reviews.llvm.org/D131606