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

Indeed there are number of mechanisms in place nowadays, so for what it's worth:

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

> Eric, asked for a little more clean up.
>
> I am aware of the following mechanisms for scheduling post ra schedulers.
>
> 1- The new virtual function in this patch


I consider overriding methods in TargetPassConfig to be the most canonical way for targets to influence pass scheduling. So this should stay.

> 2- targetSchedulesPostRAScheduling

This seems redundant to me as soon as 1) is implemented.

> 3- enablePostRA method in the subtarget
>  4- PostRAScheduler flag in include/llvm/Target/TargetSchedule.td

This redundancy is unfortunate, IMO we should just have the one in TargetSchedModel/TargetSchedule.td and not an extra callback in TargetSubtargetInfo. Last time I tried fixing this I ran into the problem that aarch64 reuses a scheduling model for two targets with different postrasched settings. One way to attack this would be to figure out if we may not better share the same postra setting or whether we should duplicate the scheduling model with different postra sched settings. The other thing which I would like to see longer term is having a way to subclass TargetSchedModel so targets can specialize a few more scheduling related functions (there is some functions like shouldScheduleAdjacent() or isAsCheapAsMove() which I think would make more sense as part of a per-subtarget scheduling model). I'd really like to see some cleanups here but don't think we should block this patch (at least not on this specific point).

> 5- command line option to turn on new scheduler instead of old one.
>  6- command line option to turn off post ra schedulers

I don't think we need to spend too much time thinking about our debugging commandline options. And those two are fine IMO.

> 7- SubstitutePass to use new scheduler instead of old one.

I never liked this mechanism. I believe it should be possible to (re-)model TargetPassConfig in a way that there is a method that can be overridden to replace a pass. (I also believe that we could simplify target passmanager configuration by doing the nonintuitive move of actually duplicating some of the setup code across our targets so they have an easier time creating variants/deviating from the default, but I can see this becoming a controversial debate I'd like to not start in this review ;-) )

> Each of these seems to have a slightly different usecase. If there is a likelihood that we can remove some of these, it might be worthwhile to open a PR. otherwise we can leave it as is.

A PR or even patches would be apreciated.

> Matt: Sorry I just realized I could have applied your comment to this update.  I will make sure to check those before committing though.


https://reviews.llvm.org/D25482