[PATCH] D22194: Power9 - Add exploitation of oris/ori fusion

Nemanja Ivanovic via llvm-commits llvm-commits at lists.llvm.org
Mon Jul 11 05:43:40 PDT 2016


nemanjai added inline comments.

================
Comment at: lib/CodeGen/PostRASchedulerList.cpp:561
@@ -552,1 +560,3 @@
 
+  assert(!NextClusterSucc && "Incorrect scheduling state.");
+
----------------
I am probably missing something, but I don't see any intervening code that might set NextClusterSucc between this assert that ensures it is not set and the code that queries it.

================
Comment at: lib/Target/PowerPC/PPCMacroFusion.cpp:17
@@ +16,3 @@
+/// Returns true if \p MI reads a register written by \p Other.
+static bool hasDataDep(const TargetRegisterInfo &TRI, const MachineInstr *MI,
+                       const MachineInstr *Other) {
----------------
I may be mis-remembering what was actually done, but I thought there was some work recently to convert functions that take MachineInstr by-pointer to by-reference when they can't be null (which I think is the case here).

================
Comment at: lib/Target/PowerPC/PPCMacroFusion.cpp:31
@@ +30,3 @@
+static void
+createClusterEdge(ScheduleDAGInstrs *DAGInstrs, SUnit &First, SUnit &Second,
+                  bool HasDependency) {
----------------
Can you add a comment about what this function does (including describing the parameters)?

================
Comment at: lib/Target/PowerPC/PPCMacroFusion.cpp:64
@@ +63,3 @@
+    MachineInstr *MI = SU[Idx].getInstr();
+    MI->dump();
+
----------------
Left over from debugging?

================
Comment at: lib/Target/PowerPC/PPCMacroFusion.cpp:66
@@ +65,3 @@
+
+    switch (MI->getOpcode()) {
+    case PPC::ORIS8 :
----------------
Since this switch statement will grow and I assume that we will perform the same action on all pairs, might it be a cleaner implementation to write a function to query if an opcode pair can be fused?
Something along the lines of:

```
bool areOpcodesFusable(unsigned Opcode1, unsigned Opcode 2) {
  static unsigned P9FusedOpcodes[] = {
    PPC::ORIS8, PPC::ORI8,
    PPC::ORIS,  PPC::ORI,
    //...
    0
  }
  for (unsigned i = 0; P9FusedOpcodes[i]; i += 2)
    if (P9FusedOpcodes[i] == Opcode1 && P9FusedOpcodes[i+1] == Opcode2)
      return true;
  return false;
}
```
Then in this function, we would just call:

```
if(areOpcodesFusable(MI->getOpcode(), SU[Idx + 1].getInstr()->getOpcode())
  // ...
```
Of course, you might also want to take the actual instructions as arguments to areOpcodesFusable so that you would only return true if the requisite data dependency exists. It just seems like it would be easier to maintain a mapping this way.

================
Comment at: test/CodeGen/PowerPC/fusing-constant.ll:16
@@ +15,3 @@
+; CHECK-P9: oris
+; CHECK-P9: ori
+; CHECK-P9: oris
----------------
Don't we want the fused instructions to be checked with CHECK-NEXT?


http://reviews.llvm.org/D22194





More information about the llvm-commits mailing list