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

[Andrew Trick via Phabricator via llvm-commits]

atrick added a comment.

General thoughts:

- Factoring code textually across components is a non-goal in itself. The resulting abstraction is often less clear and harder to maintain. It's always tempting to factor code that appears repetitious but isn't actually a serious maintainability problem. Code factoring should be driven by logical boundaries, not textual repetition.

- Inheritance as a code reuse strategy usually results in less clarity and less maintainability. I prefer object composition or free standing functions and only use inheritance when polymorphism is required.

(I know that's not very helpful advice without examples, so just take it FWIW.)

MachineScheduler thoughts:

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

- Breaking up the top-level tryCandidate and moving the boilerplate and pesky details into smaller helpers looks good.

So, on these points, the patch is a positive contribution.

- 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.

See the problem that inheritance creates? As a code reuse strategy, it violates the decoupling between Target and CodeGen.

In particular, arbitrarily gouping the heuristics into RegPressure vs. RegPressure2 and Latency vs. Latency2 is unhelpful. Each heuristic entry point that you expose to the Target should have clear semantics that aren't likely to change as GenericScheduler evolves. The contract for each entry point should be clear.

So, for example, tryCandidateClusteredWeak makes sense. It isn't going to be reimplemented as something else. You can't do this for "Latency" and "RegPressure" because that could mean a number of different things. I think the challenge here is to come up with a name for each heuristic that makes sense to expose outside of the GenericScheduler implementation.

You do not *need* to expose all heuristics used by the GenericScheduler directly to targets. It's not hard for targets to copy the few lines of code for a heuristic. Copying the code doesn't create a maintenance problem, it solves one. Just expose the heuristics that are clear and easy to describe.


https://reviews.llvm.org/D43329