[PATCH] D34144: [CodeGen] Add generic MacroFusion pass.

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: include/llvm/CodeGen/MacroFusion.h:23-27
+/// SecondMI may be part of a fused pair at all.
+typedef std::function<bool(const TargetInstrInfo &TII,
+                           const TargetSubtargetInfo &TSI,
+                           const MachineInstr *FirstMI,
+                           const MachineInstr *SecondMI)>  ShouldSchedulePredTy;
----------------
You could make `SecondMI` a reference now to indicate it is never nullptr, also saves you an assert() below.


================
Comment at: include/llvm/CodeGen/MacroFusion.h:29-34
+/// \brief Create a DAG scheduling mutation to pair instructions back to back
+/// for instructions that benefit according to the target-specific
+/// shouldScheduleAdjacent predicate function.
+std::unique_ptr<ScheduleDAGMutation>
+createMacroFusionDAGMutation(ShouldSchedulePredTy shouldScheduleAdjacent,
+                             bool FuseBlock, bool FuseExit);
----------------
How about splitting this into two functions
`createBranchMacroFusionDAGMutation()` and `createMacroFusionDAGMutation()` (or similar names) instead of the two bool parameters to make the calling code easier to read?


================
Comment at: lib/CodeGen/MacroFusion.cpp:37
+  bool FuseExit;
+  bool scheduleAdjacentImpl(ScheduleDAGMI *DAG, SUnit &AnchorSU);
+
----------------
DAG cannot be nullptr, so use a reference; Contrary to `apply()` this is an internal function which we can change easily.


================
Comment at: lib/CodeGen/MacroFusion.cpp:65
+bool MacroFusion::scheduleAdjacentImpl(ScheduleDAGMI *DAG, SUnit &AnchorSU) {
+  // For now, assume targets can only fuse with the branch.
+  const MachineInstr *AnchorMI = AnchorSU.getInstr();
----------------
Not true anymore.


================
Comment at: lib/CodeGen/MacroFusion.cpp:67-68
+  const MachineInstr *AnchorMI = AnchorSU.getInstr();
+  if (!AnchorMI || AnchorMI->isPseudo() || AnchorMI->isTransient())
+    return false;
+
----------------
I don't think we should have this shortcut:
- `AnchorMI == nullptr` should only be possible for ExitSU, which we could guard against in  `apply()`
- Hiding `isTransient()` seems unnecessary.
- Hiding `isPseudo()` seems bad. I can easily imagine cases where the last instruction of a pseudo expansions fuses with the next instruction.


================
Comment at: lib/CodeGen/MacroFusion.cpp:71-72
+  // Check if the anchor instr may be fused.
+  if (!shouldScheduleAdjacent(*DAG->TII, DAG->MF.getSubtarget(),
+                              nullptr, AnchorMI))
+    return false;
----------------
Maybe factor out `DAG->TII` and `DAG->MF.getSubTarget()` into a variable, it's used two or three times.


================
Comment at: lib/CodeGen/MacroFusion.cpp:77-79
+    // Ignore dependencies that don't enforce ordering.
+    if (Dep.isWeak())
+      continue;
----------------
Maybe we should even go for `if (Dep.getKind() != Data)` at least I can't imagine right now how anti or output dependencies could lead to macrofusion.


================
Comment at: lib/CodeGen/MacroFusion.cpp:83-85
+    const MachineInstr *DepMI = DepSU.getInstr();
+    if (!DepMI || DepMI->isPseudo() || DepMI->isTransient())
+      continue;
----------------
Similar reasoning as above; Except for `DepMI == nullptr` being a real possibility for the EntrySU which we can hit here, though you may switch to checking for `DepSU.isBoundaryNode()` insteadof `getInstr() == nullptr` to make this fact more obvious.


================
Comment at: lib/CodeGen/MacroFusion.cpp:91-124
+    // Create a single weak edge between the adjacent instrs. The only effect is
+    // to cause bottom-up scheduling to heavily prioritize the clustered instrs.
+    DAG->addEdge(&AnchorSU, SDep(&DepSU, SDep::Cluster));
+
+    // Adjust the latency between the anchor instr and its
+    // predecessors.
+    for (SDep &IDep : AnchorSU.Preds)
----------------
Extract the part that adds the fusion edge and does the adjustments into a separate function? I could imagine this being of value if it turns out a new target can use a different/special search strategy for finding fusion opportunities.


================
Comment at: lib/CodeGen/MacroFusion.cpp:131
+
+} // end namespace
+
----------------
`end anonymous namespace`


https://reviews.llvm.org/D34144