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

[Joshua Cranmer via Phabricator via llvm-commits]

jcranmer-intel requested changes to this revision.
jcranmer-intel added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:71-75
+#include "llvm/Analysis/AliasAnalysis.h"
+#include "llvm/Analysis/AliasSetTracker.h"
+#include "llvm/Analysis/BasicAliasAnalysis.h"
+#include "llvm/IR/DiagnosticInfo.h"
+#include "llvm/Transforms/Utils/LoopUtils.h"
----------------
Nit: these include lines should be sorted as appropriate in the main include block above.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:982-984
+            }
+            // Execute the hoist/sink operations on preheader instructions
+            movePreheaderInsts(*FC0, *FC1, SafeToHoist, SafeToSink);
----------------
This will unconditionally do the code motion even if (future) profitability checks decide that it's not beneficial to fuse the loops. I think it would be better to hold off on doing so until after we've checked fusion profitability, especially because not doing so means we would get the changed flag wrong in some cases.


================
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");
----------------
I'm convinced this check isn't correct, although I am still trying to figure out which check is the correct one.


================
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)) {
----------------
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.


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

https://reviews.llvm.org/D118076