[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 inline comments.


================
Comment at: include/llvm/CodeGen/MachineScheduler.h:894-895
                  SchedBoundary *OtherZone);
 
+  bool tryLatency(SchedCandidate &TryCand, SchedCandidate &Cand,
+                  SchedBoundary &Zone);
----------------
jonpa wrote:
> 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?
> 
IMO with too much subclassing and overriding, it can be quite complicated to see what's going on - but I think that comes down to personal preference, mostly. And that's just me, I suppose with those kinds of things @MatzeB  and @atrick 's thoughts are much more important ;) 

I also think it would be nice to have all heuristic functions somewhere together, to make it easier to keep track of them.


================
Comment at: include/llvm/CodeGen/MachineScheduler.h:981
+                                   SchedBoundary *Zone);
+  virtual void tryCandidate(SchedCandidate &Cand, SchedCandidate &TryCand,
+                            SchedBoundary *Zone);
----------------
jonpa wrote:
> 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?
> 
> How do you picture SystemZ adding this simple heuristic (see top of page) while reusing all else?

They do not have to be member functions, they could be regular exported functions. They don't access the scheduler object (I think), so I don't think they would need to be member functions. To me, it seems simpler not to couple the scheduler and the heuristics too much, but that's again mostly personal perference.


https://reviews.llvm.org/D43329