[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:
> 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.
I tried adding const to all the new methods, which seemed to increase readability at least to me.

tryLatency() is used both pre- and post-RA, so it needs to be in the base. I guess tryGreater() and other such small methods should also all go in the base class.

Personally, I think it makes sense to have these type of functions as part of the class, since after all that is a scheduling strategy class for which these methods actually are very central. But like you said as well, that's just me...



================
Comment at: include/llvm/CodeGen/MachineScheduler.h:981
+                                   SchedBoundary *Zone);
+  virtual void tryCandidate(SchedCandidate &Cand, SchedCandidate &TryCand,
+                            SchedBoundary *Zone);
----------------
fhahn wrote:
> 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.
I tried removing the 'static' from such a function in MachineScheduler.cpp, and then adding an 'extern' declaration in SystemZMachineScheduler.cpp, but that lead to a lot of linker errors. How would you do this?



https://reviews.llvm.org/D43329