[PATCH] D87071: [Scheduling] Add a mutation to schedule GOT indirect instructions close to each other for linker optimization

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

The fact that we catch all these additional opportunities is great. I don't fully understand all the code in the patch because I am not all that familiar with the scheduler so I'd like @stefanp to have a look at this as well.
I do have some nits inline and I'd like to see some tests that exercise the code paths where we can't reorder instructions to allow the optimization (i.e. cases where we return `nullptr` from `getPLDpcAndLdStPair()` as well as cases where we are unable to add the edges in `schedHazardInsts()`).



================
Comment at: llvm/lib/Target/PowerPC/PPCMachineScheduler.cpp:165
+// are scheduled before the PLDpc.
+void PPCLinkerOptMutation::schedHazardInsts(ScheduleDAGInstrs &DAG,
+                                            SUnit &PLDpc, SUnit &LdSt) {
----------------
It seems to me like this function could be documented a bit better to explain not only what we are doing but why. Of course, this may just be because I am not familiar with the scheduler so the code isn't self-documenting to me.


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineScheduler.cpp:276
+    } else if (Succ.getKind() == SDep::Output)
+      // Output dependency means it is redefined, which is not allowed by
+      // linker opt.
----------------
Sorry, I don't think this comment is clear enough. What is redefined? The concept of redefinition seems at odds with SSA.


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineScheduler.cpp:292
+  // stored for a store.
+  if (!SingleUseMI->getOperand(2).isReg() ||
+      SingleUseMI->getOperand(2).getReg() != DefMO.getReg())
----------------
This appears to make the assumption that operand 2 is the base address register. The comment should probably mention that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87071