[PATCH] D43329: [SystemZ, MachineScheduler] Refactor GenericScheduler::tryCandidate() to reuse parts in a new SystemZ scheduling strategy.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a reviewer: MatzeB.
fhahn added a comment.



> Refactor tryCandidate() into digestible parts so that a target that wants to override this method with minor modifications can do so easily. I tried to do a separation into the logical parts, but I am of course open to alternatives, including name changing of the new methods.

Thanks for picking this up! This is along the lines what I outlined in the email thread and what I was planning to do,  but did not find the time so far.

I left some inline comments and I think it would be best if you could split this up into a MachineScheduler patch and a SystemZ patch. One advantage of having target specific schedulers IMO should be that target maintainers can more easily accept changes without worrying about other targets.

> Coming back to the original mail discussion, this will perhaps be somewhat awkward to maintain over time - in order to keep up with the developments in the base class, one has to 1) diff both tryCandidate(), and also 2) check what calls to DAG->addMutation() are done in the generic createGenericSchedLive().

I do not think this will  be a huge issue, as there are not many changes to the generic heuristics (and with more target-specific schedulers, I expect even less need to tweak the generic heuristics). As for DAG mutations, at least the AArch64 backend already manages that itself and I do not think this has been a problem so far.



================
Comment at: include/llvm/CodeGen/MachineScheduler.h:894-895
                  SchedBoundary *OtherZone);
 
+  bool tryLatency(SchedCandidate &TryCand, SchedCandidate &Cand,
+                  SchedBoundary &Zone);
----------------
I am not sure what the benefit of that is and at the moment it seems like one setting too much to me. I think it is not unreasonable to expect people to provide their own tryCandidate implementation if they want custom latency heuristics.



================
Comment at: include/llvm/CodeGen/MachineScheduler.h:969
 
-  void tryCandidate(SchedCandidate &Cand,
-                    SchedCandidate &TryCand,
-                    SchedBoundary *Zone);
+  bool tryCandidate_RegPress(SchedCandidate &Cand, SchedCandidate &TryCand,
+                             SchedBoundary *Zone);
----------------
Usually camel case is used for function names, so I think it would be better to have tryCandidateRegPressure - same for  other methods below.



================
Comment at: include/llvm/CodeGen/MachineScheduler.h:981
+                                   SchedBoundary *Zone);
+  virtual void tryCandidate(SchedCandidate &Cand, SchedCandidate &TryCand,
+                            SchedBoundary *Zone);
----------------
I think for now it seems like making tryCandidate a virtual function gives peopl eenough freedom to ship their own heuristics while re-using most of the existing bits. I don't see the benefit of making the helper functions class members however.

Also, tryCandidate should not modify the scheduler state, so I think it should be `const`.

And as this is part of the interface now and we expect people to extend it, it would be good to document it.


================
Comment at: lib/CodeGen/MachineScheduler.cpp:2602
+bool GenericSchedulerBase::
+tryLatency(SchedCandidate &TryCand, SchedCandidate &Cand,
+           SchedBoundary &Zone) {
----------------
Splitting the code into multiple helper functions would be great opportunity to document the heuristics :)


https://reviews.llvm.org/D43329