[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 a comment.

> Creating small utility functions that can be easily combined into a useful scheduling strategy for targets is great.

I followed Florians and your suggestion and simply removed the static keyword and put the declaration in the header file. I then realized I had to pass some member objects as arguments instead when needed. For instance, tryCandidate_RegPress() calls DAG->isTrackingPressure(), which is a ScheduleDAGMILive method. Since the SchedBoundary *Zone argument only has a ScheduleDAGMI *DAG member, it cannot be used directly. Also, tryCandidate_Latency() needs the Rem object passed.

Regardless of which utility functions we end up with, I suppose this is is acceptable and preferred still to inheritance?

> An important design principle is that when GenericScheduler's implementation changes it should *not* affect targets that have already been tuned and are overriding the scheduling strategy.

Just to express my thoughts: I see this point in the sense that if a target truly had a perfect set-in-stone tuning, it would be disastrous to change anything in an uncontrollable way. But given that mischeduler is relatively new and evolving, and a target may merely have been able to improve benchmarks with a minor modification, I think it's more natural to think that a target would really want to be in on the improvements to come in the future. In other words, *not* to decouple. Take register pressure, for instance. I don't think a target that does not have it's own register pressure heuristics would want to fall behind if the common code changes in the future. That change should then be general goodness, or it should be put in a target specific strategy, right?

So to me personally, working on just one backend, it would be slightly preferred to have e.g. the tryCandidate_RegPress() function in the target strategy, so that if somebody improved it, my target would immediately get that improvement. I don't want to decouple this, since I was merely adding a heuristic with lesser priority.

On the other hand, providing just the smaller utility functions and then doing a copy-and-paste of tryCandidate() should probably work quite well in practice, as you say. In the very long run, this would also give maximum flexibility for each target.     
I suppose then we could just have one or two of those new methods, like tryCandidate_Clustered_Weak().

BTW, I am still open to suggestions on the SystemZ specific issue of answering the question "does this SU use MCProcResource X?" -- see top of page.


https://reviews.llvm.org/D43329