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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

I will have to look into the details later.

> This patch looks fine to me, but @atrick felt that MacroFusion should belong to the target.

I did some of the pushing away from the previous shared macrofusion code in the MachineScheduler. I was mainly opposed at the previous static structure that pushed all macrofusion through a TII callback. This way all targets were force to share the same logic: X86 which only does macrofusion for branches would still need to look at all instructions of the basic block. This change brings this problem back I believe?

Having said that code sharing is of course a good idea. There are obviously pieces of macrofusion that can and should be shared. Some notes on the patch though I haven't read it in too much detail:

- Is there a way to split this into smaller utility functions/classes so X86 can keep just checking the ExitSU for opportunities?
- That target callback really needs some documentation if it is intended to be reused. I find the current behavior where you sometimes call it with `SecondMI  == nullptr` and sometimes you do not confusing; It feels like two interfaces being mixed together in a single function. At the very least this needs some documentation, better yet simplify the interface if possible.



================
Comment at: include/llvm/CodeGen/MacroFusion.h:9-12
+//
+// This file contains the definition of the DAG scheduling mutation to pair
+// instructions back to back.
+//
----------------
Should use `/// \file`


================
Comment at: include/llvm/CodeGen/MacroFusion.h:19-21
+//===----------------------------------------------------------------------===//
+// MacroFusion - DAG post-processing to encourage fusion of macro ops.
+//===----------------------------------------------------------------------===//
----------------
Isn't this just repeating the \file comment above?


================
Comment at: include/llvm/CodeGen/MacroFusion.h:36
+
+} // llvm
----------------
`// end namespace llvm`


================
Comment at: lib/CodeGen/MacroFusion.cpp:10-11
+//
+// \file This file contains the implementation of the DAG scheduling mutation
+// to pair instructions back to back.
+//
----------------
Needs three slashes to be recognized by doxygen.


================
Comment at: lib/CodeGen/MacroFusion.cpp:142
+
+std::unique_ptr<ScheduleDAGMutation> createMacroFusionDAGMutation (ShouldSchedulePredTy shouldScheduleAdjacent) {
+  return EnableMacroFusion ? make_unique<MacroFusion>(shouldScheduleAdjacent) : nullptr;
----------------
no space after function name


https://reviews.llvm.org/D34144