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

[Aaron K via Phabricator via llvm-commits]

aaronkintel marked 2 inline comments as done.
aaronkintel added a comment.

In D131606#3747037 <https://reviews.llvm.org/D131606#3747037>, @congzhe wrote:

> The changes made in this patch, notably `canHoistInst()` and `canSinkInst()`  seems to have duplicate functionality from `CodeMoverUtils.cpp`. I'm wondering if we can reuse the code from `CodeMoverUtils.cpp`? Or is there something that `canHoistInst()` and `canSinkInst()` can do but functions from `CodeMoverUtils.cpp` cannot do?

The difficulty I see with utilizing `isSafeToMove{Before, After}()` from `CodeMoverUtils` in this context is that when determining if an instruction in the preheader of `FC1` can be sunk/hoisted, we need to take into account other instructions in the preheader of `FC1` which are eligible for sinking/hoisting.

Consider this example:

  define void @sink_preheader(i32 %N) {
  pre1:
    br label %body1
  
  body1:  ; preds = %pre1, %body1
    ...
    br i1 %cond, label %body1, label %pre2
  
  pre2:
    %i1 = add i32 1, %N
    %i2 = add i32 1, %i1
    br label %body2
  
  ....
  }

Both `%i1` and `%i2` can be hoisted, though `isSafeToMoveBefore()` would presumably not believe `%i2` should be moved to the end of `pre1`. If you're seeing something I'm not, let me know and I'd be happy to simplify the code.



================
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:
> 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?
In fact, it should still work without any changes. Will fix


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

https://reviews.llvm.org/D131606