[PATCH] D61930: [PowerPC] Add a specific heuristic to schedule the addi before the load

[Jinsong Ji via Phabricator via llvm-commits]

jsji requested changes to this revision.
jsji added a comment.
This revision now requires changes to proceed.

I think we should update the title or summary to indicate that this is P9 <https://reviews.llvm.org/P9> only.



================
Comment at: llvm/lib/Target/PowerPC/PPCMachineScheduler.cpp:12
 
+static bool isADDIInstr(const MachineInstr &Inst) {
+  return Inst.getOpcode() == PPC::ADDI || Inst.getOpcode() == PPC::ADDI8;
----------------
Can this be a lamda or std::function?


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineScheduler.cpp:32
+  else {
+    if (isADDIInstr(*Cand.SU->getInstr()) &&
+        TryCand.SU->getInstr()->mayLoad()) {
----------------
These logic are similar to above, just swapped the Candidates. 
Can we do it something like:
```
Cand1st= Zone.isTop() ? TryCand.SU->getInstr(): Cand.SU->getInstr();
Cand2nd= Zone.isTop() ? Cand.SU->getInstr(): *TryCand.SU->getInstr();

if(isADDIInstr(*Cand1st)) && Cand2nd->mayLoad())){
...
}
..
```


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineScheduler.cpp:61
+  // latency, as RA may create a true dependency between the load and addi.
+  if (tryAddiLoadCandidate(Cand, TryCand, *Zone))
+    return;
----------------
Can we add an option to disable this heuristic?


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineScheduler.h:29
+private:
+  bool tryAddiLoadCandidate(SchedCandidate &Cand,
+                            SchedCandidate &TryCand,
----------------
I am seeing this as a bias heuristic, so maybe `biasAddi`?


================
Comment at: llvm/test/CodeGen/PowerPC/schedule-addi-load.mir:1
+# RUN: llc -mcpu=pwr9 -mtriple powerpc64le-unknown-linux-gnu -start-before machine-scheduler -stop-after machine-scheduler -verify-machineinstrs %s -o - | FileCheck %s
+
----------------
Can we commit this testcase as a NFC before this patch? So that the only difference in this patch will be the scheduling change. Thanks.


================
Comment at: llvm/test/CodeGen/PowerPC/schedule-addi-load.mir:2
+# RUN: llc -mcpu=pwr9 -mtriple powerpc64le-unknown-linux-gnu -start-before machine-scheduler -stop-after machine-scheduler -verify-machineinstrs %s -o - | FileCheck %s
+
+# Test that if the scheduler moves the addi before the load.
----------------
We can also add a test for the newly added option to disable this heuristic


================
Comment at: llvm/test/CodeGen/PowerPC/schedule-addi-load.mir:3
+
+# Test that if the scheduler moves the addi before the load.
+--- |
----------------
Add a line as a negative test for pwr8 , as it doesn't use FeaturePPCPreRASched.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61930/new/

https://reviews.llvm.org/D61930