[PATCH] D36704: [CodeGen] Improve the consistency of instruction fusion

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: llvm/lib/CodeGen/MacroFusion.cpp:38-46
+  // Check that neither instr is already paired with another along the edge
+  // between them.
+  for (SDep &SI : FirstSU.Succs)
+    if (SI.isCluster())
+      return false;
+
+  for (SDep &SI : SecondSU.Preds)
----------------
Given the code below that adds all succs of first as succs of second, and vice versa for preds of seconds. How can the situation you are checking here even happen without us failing the cycle check in addEdge anyway?



================
Comment at: llvm/lib/CodeGen/MacroFusion.cpp:73
+    if (SI.isWeak() || (SI.getKind() != SDep::Order && SI.isCtrl()) ||
+        &SecondSU == &DAG.ExitSU || SU == &DAG.ExitSU ||
+        SU == &SecondSU || SU->isPred(&SecondSU))
----------------
Why did you move the SecondSU == ExitSU check into the loop? The condition won't change with different elements.


================
Comment at: llvm/lib/CodeGen/MacroFusion.cpp:82-94
+  // Make the FirstSU also dependent on the dependencies of the SecondSU to
+  // prevent them from being scheduled between the FirstSU and the SecondSU.
+  for (const SDep &SI : SecondSU.Preds) {
+    SUnit *SU = SI.getSUnit();
+    if (SI.isWeak() || (SI.getKind() != SDep::Order && SI.isCtrl()) ||
+        SU == &DAG.ExitSU || &FirstSU == &DAG.ExitSU ||
+        &FirstSU == SU || FirstSU.isSucc(SU))
----------------
This seems unnecessary for SecondSU == ExitSU, maybe shortcut it?


================
Comment at: llvm/lib/CodeGen/MacroFusion.cpp:145-147
     // Ignore dependencies that don't enforce ordering.
-    if (Dep.getKind() == SDep::Anti || Dep.getKind() == SDep::Output ||
-        Dep.isWeak())
+    if (Dep.isWeak() || (Dep.getKind() != SDep::Order && Dep.isCtrl()))
       continue;
----------------
I'm not sure `if (A || (!B && !C)) continue` is easier to understand than `if (A || B || C) continue`...

And I probably should have complained in D34144 already, but this comment doesn't relect reality anymore! And we should document our assumption that fusion can only happen when there is a data dependency or and Order dependency in case of physreg deps (as X86 eflags).


Repository:
  rL LLVM

https://reviews.llvm.org/D36704