Page MenuHomePhabricator

Move Schedule class to header file for allowing inheritance
Needs ReviewPublic

Authored by atheel.ma on Apr 30 2018, 12:15 AM.

Details

Summary

MachineSchedulerBase and PostRAScheduler were moved to a header file for allowing inheriting them. this will save code copying if the user want to create his own TargetPostRAScheduler and TargetMachineScheduler

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

atheel.ma created this revision.Apr 30 2018, 12:15 AM

this is my first commit suggestion in the open source. I hope the change is clear enough.

These classes are not meant to be inherited from, and inheritance is generally terrible way to achieve code reuse across components. The principle behind the scheduling interfaces is to avoid implicit coupling between the target code and machine independent code.

Are you trying to create a new pass, separate from the pre-ra and post-ra schedulers? If so, what code do you need to reuse? scheduleRegions?

atheel.ma added a comment.EditedApr 30 2018, 1:45 AM

thanks for your comment,
yes, I need to reuse "scheduleRegions" code. why its so important to avoid implicit coupling between the target code and machine independent code in this class?

I’m implementing an optimization pass analysis that will be used from the TargetHazardRecognizer, so I must pass its result in the constructor of the HazaradRecognizer.
I have encountered some open source classes that I need to expand (using inheritance) but couldn’t due to some reasons:

  1. Expanding “MachineSchedulerBase” from MachineScheduler.cpp: I want to inherit it and expand it (for changing the call of CreateTargetPostRAHazardRecognizer)but I can’t because its declared in cpp file not in header
  2. the same as above for PostRASchedule pass.
  3. Expanding BasicAAResult from BasicAliasAnalysis.h: Its inheriting the “AAResultBase<BasicAAResult>” using “Mixin”, it looks like its not designed for been inherited again.

• In BasicAAResult I have added “CheckBankConflict” functions that uses functions

from this class (need to re-design it into TargetBankConflict.cpp)

• In MachineSchedulerBase I want to change the call for the HazardRecognizer. I need

to pass the BasicAAResults when creating the HazaradRecognizer:
llvm::createTargetHazardRecognizer(MachineFunction &MF) (I cant created it inside the HazaradRecognizer because its not a pass)

so I have changed it into:
llvm::createTargetHazardRecognizer(MachineFunction &MF, BasicAAResult *BCA)
and I need to change the functions declaration and implementation that calls it. (functions from (class MachineScheduler : public MachineSchedulerBase ) and more…
so I want to:

  • Create TaregtBankConflict.h pass and pass its result to the CXDHazaradRecognizer. For that I will need to:

• Inherit BasicAAResult for avoiding code copy (I have no solution for it).
• Inherit MachineScheduler for avoiding code copy. (my suggestion is to move it to the header file)

Eugene.Zelenko added inline comments.Apr 30 2018, 6:35 AM
include/llvm/CodeGen/PostRASchedulerList.h
1

Please add LLVM header. Just copy from other file and change file name and description.

20

Please capitalize header guard name. Same in other places.

25

Please run clang-format.

31

Please separate with empty line.

34

Is this method frequently used? If not, move it to source file to reduce header dependencies. Same for getRequiredProperties().

57

Please remove one of empty line.

lib/CodeGen/PostRASchedulerList.cpp
77

Please use = default;

atheel.ma marked 6 inline comments as done.
  • run clang-format
  • moved member functions implementation to the cpp file
  • added header description
  • capitalized header guard name
Eugene.Zelenko added inline comments.May 1 2018, 7:32 AM
include/llvm/CodeGen/PostRASchedulerList.h
31

Are all these header necessary? You could use forward declarations for pointers/references.

Include What You Use is helpful too.

60

Please move to source file.

atrick added a comment.May 3 2018, 3:07 PM

thanks for your comment,
yes, I need to reuse "scheduleRegions" code. why its so important to avoid implicit coupling between the target code and machine independent code in this class?

I’m implementing an optimization pass analysis that will be used from the TargetHazardRecognizer, so I must pass its result in the constructor of the HazaradRecognizer.
I have encountered some open source classes that I need to expand (using inheritance) but couldn’t due to some reasons:

  1. Expanding “MachineSchedulerBase” from MachineScheduler.cpp: I want to inherit it and expand it (for changing the call of CreateTargetPostRAHazardRecognizer)but I can’t because its declared in cpp file not in header
  2. the same as above for PostRASchedule pass.
  3. Expanding BasicAAResult from BasicAliasAnalysis.h: Its inheriting the “AAResultBase<BasicAAResult>” using “Mixin”, it looks like its not designed for been inherited again.

    • In BasicAAResult I have added “CheckBankConflict” functions that uses functions from this class (need to re-design it into TargetBankConflict.cpp) • In MachineSchedulerBase I want to change the call for the HazardRecognizer. I need to pass the BasicAAResults when creating the HazaradRecognizer: llvm::createTargetHazardRecognizer(MachineFunction &MF) (I cant created it inside the HazaradRecognizer because its not a pass) so I have changed it into: llvm::createTargetHazardRecognizer(MachineFunction &MF, BasicAAResult *BCA) and I need to change the functions declaration and implementation that calls it. (functions from (class MachineScheduler : public MachineSchedulerBase ) and more… so I want to:
  4. Create TaregtBankConflict.h pass and pass its result to the CXDHazaradRecognizer. For that I will need to: • Inherit BasicAAResult for avoiding code copy (I have no solution for it). • Inherit MachineScheduler for avoiding code copy. (my suggestion is to move it to the header file)

Atheel,

CreateTargetPostRAHazardRecognizer is not called from the MachineScheduler. It uses this interface:

CreateTargetMIHazardRecognizer(const InstrItineraryData *, const ScheduleDAG *DAG) const;

It would make sense to change this interface to:

CreateTargetMIHazardRecognizer(const InstrItineraryData *, const ScheduleDAGMI *DAG) const;

ScheduleDAGMI has an optional pointer to the AliasAnalysis if it is available. If you don't feel like changing the TII interface type, you could just static_cast in your code as a workaround.

Alias analysis is supposed to be extended by layering the analyses (I've never done that with the new pass manager, so please ask on the dev list). If you need some functionality in BasicAA, then I think those functions should be exposed as utilities that you can call into, rather than subclassing BasicAA itself.

In general, it sounds like you're going about extending the machine independent code backward. Some classes are interfaces, which are designed to be overriden by targets. Other classes provide implementations that call into those interfaces. Implementaton classes should never be subclassed by targets. That would quickly lead to unmaintainable code. It should be possible to reimplement anything in-tree (open source) without breaking existing targets. Targets/subtargets can override and customize the pipeline at many different levels depending on how much you want to reimplement, without additional subclassing beyond the usual target interfaces like TargetInstrInfo. Often you find that you need to expose some functionality from machine independent code. Subclassing is never the right way to expose functionality from a library! (I don't even like the fact that GenericScheduler was moved to a header, but I guess that was the quickest way to make a utility out of it).

Andy

Hi Andy,

thanks for your suggestion and remarks.
I think passing DAGMI(ScheduleDAGInstrs) instead of DAG can be the first step for my solution, then I will also need to add some functions to the interface of AliasAnalisys (some generic functions like getDistanceBetweenMemorys(MIa,MIb)...). I hope this will be accepted.

but the problem is that AliasAnalysis *AAForDep; is 'protected' inside the DAGI, do you think its acceptable to add getAA() to the interface of the DAGMI ?

thanks,
Atheel

Hi Andy,

thanks for your suggestion and remarks.
I think passing DAGMI(ScheduleDAGInstrs) instead of DAG can be the first step for my solution, then I will also need to add some functions to the interface of AliasAnalisys (some generic functions like getDistanceBetweenMemorys(MIa,MIb)...). I hope this will be accepted.

but the problem is that AliasAnalysis *AAForDep; is 'protected' inside the DAGI, do you think its acceptable to add getAA() to the interface of the DAGMI ?

thanks,
Atheel

Adding getAA() seems harmless since it could just return nullptr for any target that doesn't use AA. You could assert that it's nonnull for your target.

Hi again.
as I see now that passing ScheduleDAGMI instead of ScheduleDAG is not a good idea,
I think ScheduleDAG is better becuase we dont want to force the user to use the implementation of the opensource (ScheduleDAGMI),
if we pass ScheduleDAG then the user can inherit from ScheduleDAG and make his own pass, but if we change it to ScheduleDAGMI (which is declared and implemented in an open source *CPP* file - meant cant inherit from it, then the user will be stuck with this implemntations of ScheduleDAGMI...

so I guess I will need to use static cast (unfortunately...)

if you have another suggestion I will be happy so hear about.

thanks

ScheduleDAGMI is defined in MachineScheduler.h. Maybe there's some other reason the API should take ScheduleDAG, but I'm not aware of it. I think that base class is only to share implementation across SelectionDAG and MI scheduling.