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

[Ehsan Amiri via llvm-commits]

amehsan added a comment.

In https://reviews.llvm.org/D25482#567561, @MatzeB wrote:

> I think I should be more specific in my critique and I noticed that I missed part of the problem while reading this the first time:
>
> - I'm against adding infarstructure/special cases at a basic level like MachineFunctionPass. It is already more complicated than it should be today.
> - The current way of replacing the PostRAScheduler with the MachinePostRAScheduler is messy indeed. How about always scheduling both passes and letting enablePostRAScheduler() return an enum value indicating whether we want the machine or the list scheduler to run?


Thanks. First of all part of the solution that you are proposing is what is being done in the patch. Both passes are being added to the pipeline. You are suggesting a somewhat different approach for each pass to decide whether it should run or not. I think using enablePostRAScheduler() pollutes subtarget classes. Details follow.

The problem is that, if I want to take this approach then:

1- Subtarget has a method devoted to turning on or off PostRA scheduling.
2- Subtarget has to be aware of different PostRA scheduling passes that we have (to be able to pass proper enum values).

The solution in this patch provides one general solution for subtargets to answer queries about whether a pass should run or not. It also relies on existing Pass IDs. If this problem pops up again for a different pass or a different platform, all we need to do is to change the platform's runPass method that I have introduced here (and add a couple of lines to it). Also the given pass will need to change the skipFunction method that it calls. (all that is needed for that, is to remove a few characters of the code, as you can see in this patch).

Also please note that the method enablePostRASchedule can be removed from the subtarget classes with the infrastructure introduced in this patch. I can open a PR so we can do that cleanup later.

In comparison, the solution proposed in this comment, requires having a method corresponding to the given pass in the subtarget AND also making subtarget aware of PassIDs or something equivalent (like enums that you are proposing).

Just to give you an example that this is not an isolated problem, I can mention modulo scheduling pass. If we want to turn that on for PPC most likely it will be only for PWR9 not earlier processors. So do we need to add a new method enableMachinePipelining to the subtarget? I think it will be cleaner to use the infrastructure introduced in this patch. Call proper overload of skipFunction and add a couple of lines to runPass method and check for the ID of that pass.

If we come to the conclusion that this is the right approach for solving this problem, the fact that MachineFunctionPass is already complicated is beside the point. If something should not be in there, it should be removed. If we need to refactor it to handle its complexity that is a different problem to tackle.


https://reviews.llvm.org/D25482