Open llvmbot opened 8 years ago
llvmbot
commented
8 years ago My follow up comment on the code review
2- targetSchedulesPostRAScheduling This seems redundant to me as soon as 1) is implemented.
Maybe I am missing something, but this seems different from (1). Note that addPostRASched is called from addMachinePasses which is not typically overriden by targets. SystemZ has a usecase for targetSchedulesPostRAScheduling which cannot be covered by (1).
llvmbot
commented
8 years ago Comment from Matthias Braun on D25482
Indeed there are number of mechanisms in place nowadays, so for what it's worth:
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.
Extended Description
I am aware of the following mechanisms for scheduling post ra schedulers.
1- The new virtual function in this patch 2- targetSchedulesPostRAScheduling 3- enablePostRA method in the subtarget 4- PostRAScheduler flag in include/llvm/Target/TargetSchedule.td 5- command line option to turn on new scheduler instead of old one. 6- command line option to turn off post ra schedulers 7- SubstitutePass to use new scheduler instead of old one.
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.
This is taken from D25482 (towards the end of conversation). I add some follow up comments from there in the comments of this PR