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

[Aaron K via Phabricator via llvm-commits]

aaronkintel added a comment.

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

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

I tried some tests with `isSafeToMoveBefore()`. The sink_preheader.ll <https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/LoopFusion/sink_preheader.ll> test fails, since `%sinkme` can be sunk but then `%sinkme2` cannot sink, because `isSafeToMoveBefore()` apparently does not ignore users in the same block like it ignores operands in the same block when hoisting. The condition `CheckForEntireBlock` for sinking could be added, to mirror the case with hoisting, but I foresee other issues. For example, `%sinkme3` in `sink_preheader.ll` should not be hoisted, though the only obstacle for hoisting it (`%sinkme1` ) is within the same block as  `%sinkme3` itself; we need to know that `%sinkme1` //also// cannot be hoisted, and it is insufficient to just ignore variable.


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

https://reviews.llvm.org/D131606