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

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

Ok, I think I see what you're getting at. The function `isSafeToMoveBefore()` could be used to sink/hoist the entire preheader of `FC1` atomically, but it cannot handle the situation where some instructions can be sunk and some can be hoisted safely.


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

https://reviews.llvm.org/D131606