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

[Chuang-Yu Cheng via llvm-commits]

cycheng marked 4 inline comments as done.

================
Comment at: lib/CodeGen/PostRASchedulerList.cpp:561
@@ -552,1 +560,3 @@
 
+  assert(!NextClusterSucc && "Incorrect scheduling state.");
+
----------------
nemanjai wrote:
> 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.
Or should I just set it to nullptr? Because we will call schedule several times, even though I thought I've processed it properly, but I just want to make sure again.

================
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) {
----------------
nemanjai wrote:
> 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).
They can't be null, I can convert to by-reference as well.

================
Comment at: lib/Target/PowerPC/PPCMacroFusion.cpp:64
@@ +63,3 @@
+    MachineInstr *MI = SU[Idx].getInstr();
+    MI->dump();
+
----------------
nemanjai wrote:
> Left over from debugging?
Oh.. I thought I've removed it .. :P

================
Comment at: lib/Target/PowerPC/PPCMacroFusion.cpp:66
@@ +65,3 @@
+
+    switch (MI->getOpcode()) {
+    case PPC::ORIS8 :
----------------
nemanjai wrote:
> 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.
Thanks, this is a good idea, I am working on a new version based on your suggestion here, it is table based design and would be able to handle our current requirements.

By the way, this design should be able to handle the following pattern in a more clear way.

1-to-1 pattern:

```
oris Rx,RA,SIh
ori Rt,Rx,SIl

xoris Rx,RA,SIh
xori Rt,Rx,SIl 

addis Rx,RA,SIh
addi Rt,Rx,SIl 
```

1-to-n pattern:

```
op1: addis Rx,RA,SIh

op2: stb RS,Rx,SIl
     sth RS,Rx,SIl
     stw RS,Rx,SIl
     std RS,Rx,SIl 
```

special requirement on op1 or op2:

```
op1: addi Rx,0,SI
op2: stdx RS,Rx,RB
```

================
Comment at: test/CodeGen/PowerPC/fusing-constant.ll:16
@@ +15,3 @@
+; CHECK-P9: oris
+; CHECK-P9: ori
+; CHECK-P9: oris
----------------
nemanjai wrote:
> Don't we want the fused instructions to be checked with CHECK-NEXT?
Yes! I should use CHECK-NEXT, thanks a lot !!


http://reviews.llvm.org/D22194