[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#3750026 <https://reviews.llvm.org/D131606#3750026>, @aaronkintel wrote:

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

IMHO I don't think `%sinkme3` will be hoisted. Maybe I should have made it more clear in my last comment: in loop fusion we would usually want to move an entire BB, for example moving the preheader of the second loop to the first loop in order to create an empty preheader. The API from `CodeMoverUtils` for this purpose is the following, where the first argument is `BasicBlock&`. Maybe it should be renamed to something like "isSafeToMoveBBBefore()" to be more clear.

  bool llvm::isSafeToMoveBefore(BasicBlock &BB, Instruction &InsertPoint,
                                DominatorTree &DT, const PostDominatorTree *PDT,
                                DependenceInfo *DI) {
    return llvm::all_of(BB, [&](Instruction &I) {
      if (BB.getTerminator() == &I)
        return true;
  
      return isSafeToMoveBefore(I, InsertPoint, DT, PDT, DI,
                                /*CheckForEntireBlock=*/true);
    });
  }

`%sinkme3` won't be hoisted because `%sinkme1` won't be hoisted at the first place since `%barrier` does not dominate the insertion point, and `llvm::all_of()` would bail out early. Therefore IIUC this would not be an issue.

I did not mean to block or suspend this patch, although I think it might be cleaner and simpler to add some code in `isSafeToMoveBefore()` to mirror the case with hoisting and then reuse `isSafeToMoveBefore()`, which seems sufficient to fullfil your purposes. I quickly looked at the logic in `isSafeToMoveBefore()` and it looks like adding some code under the `if (isReachedBefore(&I, &InsertPoint, &DT, PDT))` condition to enable stronger sinking capability might just do the work, similar to the code added under `if (isReachedBefore(&InsertPoint, &I, &DT, PDT))` that enabled stronger hoisting.

Again I'm open to any possible solution, let it be this patch or other approaches - just trying to provide some input.


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

https://reviews.llvm.org/D131606