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

[Joshua Cranmer via Phabricator via llvm-commits]

jcranmer-intel added a comment.

There's two more tests I would like to see here:

- One test that checks that an intrinsically unmovable instruction (e.g., call to an unknown function) in the preheader blocks hoisting/sinking.
- Another test (in the same file as above is possible) checks that a memory dependency blocks hoisting/sinking.



================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:954-955
+          // hoist them up to the first loop's pre-header or sink them into the
+          // body of the second loop.
+          auto CanSinkHoistPreheader{true};
+          SmallVector<Instruction *, 4> SafeToHoist;
----------------
I don't think this variable is necessary, since if it's `false`, you've already stopped looking at this loop and have continued to the next one.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1052-1053
+      SmallVector<Instruction *, 4> &SafeToHoist,
+      SmallVector<Instruction *, 4> &SafeToSink) const {
+    auto *FC1Preheader{FC1.Preheader};
+    for (auto &I : *FC1Preheader) {
----------------
Nit: As a general style guidelines, C++11 universal initializer syntax is generally disfavored, as is `auto`.

The rough guideline I use for `auto` is that you generally want types to appear exactly once, so when you're assigning the result of a cast (especially the `dyn_cast` checks), then you go ahead and use `auto`, but otherwise, you generally stay away from it.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1075
+             "Expected single successor for loop preheader.");
+      auto ShouldHostInst{true};
+      for (auto &Op : I.operands()) {
----------------
Nit: This probably meant should be named `CanHoistInst`.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1078-1079
+        if (auto *OpInst{dyn_cast<Instruction>(Op)}) {
+          const auto OpHoisted{std::find(SafeToHoist.begin(), SafeToHoist.end(),
+                                         OpInst) != SafeToHoist.end()};
+          // Check if we have already decided to hoist this operand. In this
----------------
You should be able to replace this with `bool OpHoisted = is_contained(SafeTohoist, OpInst);` (may need to include STLExtras.h).


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1082
+          // case, it does not dominate FC0 *yet*, but will after we hoist it.
+          if (!(DT.dominates(OpInst, FC0PreheaderTarget) || OpHoisted)) {
+            ShouldHostInst = false;
----------------
Nit: `OpHoisted` should precede the dominator check.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1097-1099
+            // If FC1 has phi users of this value, we cannot sink it into FC1.
+            if (FC1.L->contains(UI) ||
+                (isa<PHINode>(UI) && UI->getParent() == FC1.Header)) { // FIXME
----------------
The first half of the or statement here subsumes the second half, does it not?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFuse.cpp:1344-1346
+
+    NumHoistedInsts = HoistInsts.size();
+    NumSunkInsts = SinkInsts.size();
----------------
You mean `+=` here, not `=`.


================
Comment at: llvm/test/Transforms/LoopFusion/no_sink_hoist.ll:2-4
+; RUN: opt -S -loop-fusion -debug -o %t1 < %s &> %t2
+; RUN: cat %t1 | FileCheck %s
+; RUN: grep "Could not hoist/sink all instructions" %t2
----------------
For negative fusion checks, I think a test along the lines of llvm/test/Transforms/LoopFusion/cannot_fuse.ll is more appropriate.


================
Comment at: llvm/test/Transforms/LoopFusion/no_sink_hoist_inner_barier.ll:1
+;;;; RUN: opt -S -loop-fusion < %s | FileCheck %s
+; RUN: opt -S -loop-fusion -debug -o %t1 < %s &> %t2
----------------
Nit: Too many semicolons.

Also, the file's name should be "no_sink_hoist_inner_barrier.ll"


================
Comment at: llvm/test/Transforms/LoopFusion/sink_preheader.ll:1
+; RUN: opt -S -loop-fusion < %s | FileCheck %s
+
----------------
For this test, I'd strongly prefer to see update_test_checks.py-style checks.


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

https://reviews.llvm.org/D118076