[PATCH] D25482: [PPC] Allow two post RA schedulers to be in the pipeline and select one depending on the Machine Function's subtarget

[Matthias Braun via llvm-commits]

MatzeB added a comment.

In https://reviews.llvm.org/D25482#569399, @amehsan wrote:

> > The pattern adds an extra non-obvious possibility why a pass wouldn't run, which has its costs when reading and debugging any pass.
>
> Not sure I follow here. Note that we have two overload of skipFunction(). Any pass that does not want to call the subtarget, can call the current overload. Any pass that calls the new overload, will need to call subtarget one way or another. So the extra check is there in both cases.
>
> > There is exactly one runPass() function which has to handle a combination of subtarget + passid to make a decision, this is very likely to get unwieldy as more passes and more subtarget exceptions get added in the future.
>
> In most cases this function looks at (cpu, pass) pair and return true/false based on that. Any subtarget that starts to have a complicated runPass, can engineer their code properly. For example by adding private functions for each pass, and calling the private function based on pass id.  (Note that the difference with having enablePostRA... function is that the pass dependent extra function is no longer part of the subtarget interface).
>
> > Having 1 callback per pass (like enablePostRAScheduling) at least keeps the passid dimension out of it and makes it easier to search and understand the behaviour IMO.
>
> I see two problems with this. The minor problem is this: for the example that we have discussed so far, we need one enablePostRA function that returns an enum and an enableMachinePipeliner that returns a bool. This is a minor advantage for the current patch in terms of code quality. (In addition to a less polluted interface for subtarget). The more important problem is related to the following:


I don't see the code becoming cleaner here. Instead of a targeted patch that just affects the behavior of PostMachineScheduler and PostRASchedulerList you add a generic mechanism that affects every pass calling skipFunction(). I don't see the majority of passes using this functionality.
So what you see as helpful/useful utility for other passes, mainly strikes me as a (small but not neglibile) increase in accidental complexity.

>> Well I was proposing to always schedule both passes in TargetPassConfig rather than requiring interested targets to overload a newly introduced addPostRASched() functions to keep things simpler.
> 
> This is not a good idea. There are already backends that have stopped using Post RA top down scheduler <https://github.com/llvm-mirror/llvm/blob/master/lib/Target/AArch64/AArch64TargetMachine.cpp#L289> and use the new MI scheduler. Also the general idea, AFAIK, is that the top down scheduler is the old mechanism and MI scheduler is the new mechanism. With what you are proposing, we are saying that people who have moved/will move to the new scheduler (or potentially any new subtarget that starts using new mechanism from the beginning), should continue calling the old scheduler until every backend has moved off of it so we can entirely remove it. I don't think this is a good idea. This reminds me of the first point of your comment that I quoted here: "The pattern adds an extra non-obvious possibility why a pass wouldn't run, which has its costs when reading and debugging any pass.". Not exactly the same issue, but similar.

I don't see the point. This is one more pass scheduled that most likely immediately boils out at the beginning of runOnMachineFunction(), I don't see how that is worse than adding one more `if()` to every MachinePass. Sure I would love to not have to schedule two passes, so go ahead and upgrade your subtargets PPC! Until then I am fine with the simple solution.


https://reviews.llvm.org/D25482