[PATCH] D118076: Sinking or hoisting instructions between loops before fusion

[Aaron K via Phabricator via llvm-commits]

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

The build failures are all different, and are evidently unrelated to this change.



================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1064-1065
+      // additional work with a dependence analysis so for now we give
+      // up on memory reads.
+      if (I.mayWriteToMemory() || I.mayThrow() || I.mayReadFromMemory()) {
+        LLVM_DEBUG(dbgs() << "Inst: " << I << " has side-effects.\n");
----------------
jcranmer-intel wrote:
> I'm convinced this check isn't correct, although I am still trying to figure out which check is the correct one.
Let me know what you thi


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1081-1084
+      // changing LICM code which would result in more side-effects.
+      if (!isa<DbgInfoIntrinsic>(I) &&
+          !canSinkOrHoistInst(I, &AA, &DT, FC0.L, &CurAST0, nullptr, true) &&
+          !canSinkOrHoistInst(I, &AA, &DT, FC1.L, &CurAST1, nullptr, true)) {
----------------
jcranmer-intel wrote:
> Nevertheless, the specific checks you used above generally does not comport with these checks. The first set of checks bans all memory instructions from being checked, and if you drill into canSinkOrHoistInst, almost all of the checks in there are about verifying correctness of memory instructions, with the residual checks of call instructions in large part anyways not being what you wanted anyways, given your choice to override its decision point for `DbgInfoIntrinsic`.
> 
> Offhand, `isSafeToSpeculativelyExecute` feels like it's the best *starting* point for correctness checks for hoisting. This doesn't cover the dependency checking or domination checks, but you can look at `isSafeToMoveBefore` in `CodeMoverUtils.cpp` to understand the general basis for the decision point. If we had a `isSafeToMoveAfter`, I'd outright just say that using a combination of `isSafeToMoveBefore` and `isSafeToMoveAfter` is the best way to go, but we don't, so it would be worth digging into `isSafeToMoveBefore` anyways to understand its machinery.
I have taken a different approach here by using domination for hoisting and checking the users for sinking. Is this satisfactory?


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

https://reviews.llvm.org/D118076