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

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added a comment.

In D131606#3747152 <https://reviews.llvm.org/D131606#3747152>, @aaronkintel wrote:

> 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.

IIUC the problem you described has been addressed and handled in `CodeMoverUtils` already.

The function `isSafeToMoveBefore()` has the following signature where the last argument `CheckForEntireBlock` can be set to true such that previous instructions (before the current instruction that is under consideration) would be considered when checking if it is legal to move it. If I'm not mistaken, for the IR you posted we can actually move both `%i1` and `%i2` (by setting `CheckForEntireBlock=true`). You might want to check out this patch https://reviews.llvm.org/D110378, which introduced `CheckForEntireBlock`.

  bool isSafeToMoveBefore(Instruction &I, Instruction &InsertPoint,
                          DominatorTree &DT,
                          const PostDominatorTree *PDT = nullptr,
                          DependenceInfo *DI = nullptr,
                          bool CheckForEntireBlock = false);

If I'm not mistaken (please correct me if I'm wrong), I wish we could reuse `CodeMoverUtils` since it could simplify things.


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

https://reviews.llvm.org/D131606