[PATCH] D46243: Move Schedule class to header file for allowing inheritance

[Andrew Trick via Phabricator via llvm-commits]

atrick added a comment.

In https://reviews.llvm.org/D46243#1082447, @atheel.ma wrote:

> 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


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D46243