[PATCH] D86684: [NFC][Refactor] Add the SchedHeuristic for Scheduler to allow platform customizing the heuristics

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang added a comment.

> Hello. This looks interesting. I've run into places before where we just wanted to add an extra heuristics. I think some of our downstream schedules go a bit beyond this and would still need to override tryCandidate (the merge is going to be a pain!), but I've run into situations where it would definitely be useful.

Yes, our downstream scheduler also has the same pain. And what we can do now is to duplicate the tryCandidate and update it, which is hard to maintain. See D86681 <https://reviews.llvm.org/D86681> .

> This does add some non-inlineable virtual dispatch to a rather hot part of the compiler though. Do you have compile time numbers, to make sure this isn't going to cause any problems?

I will take this concern into consideration and evaluate the compiling time.



================
Comment at: llvm/include/llvm/CodeGen/MachineScheduler.h:1099
 
+private:
+  // Keep all the heuristics. Notice that, the earlier it is pushed into the
----------------
dmgreen wrote:
> I would personally just keep everything as protected.
We have to declare the DAG as private as the subclass GenericScheduler/PostGenericScheduler both already keep it with different type. For the Heuristics, yes, we can make it as protected.


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineScheduler.h:56-59
   void initialize(ScheduleDAGMI *Dag) override;
   SUnit *pickNode(bool &IsTopNode) override;
   void enterMBB(MachineBasicBlock *MBB) override;
   void leaveMBB() override;
----------------
dmgreen wrote:
> None of these seems to do anything, beyond calling the base class. It's almost a shame to have to create a subclass of PostGenericScheduler just to add an extra heuristic. As opposed to being able to register them like mutations.
> 
> I imagine that a lot of the time these would end up doing something though and the scheduler would end up holding some state. That would be the case for our downstream schedulers I think. So I think this is fine as-is, but you might want to think a bit about how it would be possible to register without needing to subclass.
Good suggestion. I think, we can keep the state inside the heuristic and add some necessary interface to initialize the state, update the state, and compare the candidate. Then, we can do it the same as mutation without subclass the scheduler. I will try with this direction.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86684/new/

https://reviews.llvm.org/D86684