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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: include/llvm/CodeGen/MachineScheduler.h:894-895
                  SchedBoundary *OtherZone);
 
+  bool tryLatency(SchedCandidate &TryCand, SchedCandidate &Cand,
+                  SchedBoundary &Zone);
----------------
fhahn wrote:
> 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.
> 
Well... why have duplicated code in a Target backend, if it can be helped?

To me, it is generally reasonable to assume that a target starts out with enabling the generic mischeduler, and then as a next step experiment with it by adding / removing / reordering heuristics. What harm is it then to provide the means to do so with protected member methods?



================
Comment at: include/llvm/CodeGen/MachineScheduler.h:981
+                                   SchedBoundary *Zone);
+  virtual void tryCandidate(SchedCandidate &Cand, SchedCandidate &TryCand,
+                            SchedBoundary *Zone);
----------------
fhahn wrote:
> 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.
How are those bits going to be reused if not by inheritance?

How do you picture SystemZ adding this simple heuristic (see top of page) while reusing all else?



https://reviews.llvm.org/D43329