[PATCH] D52932: [MCSched] Bind PFM Counters to the CPUs instead of the SchedModel.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Thanks for waiting Clement.

I am now back from holiday, and I finally had time to look at your patch.

What concerns me about this approach is that MCSubtargetInfo describes a processor for the purpose of codegen.
At least for me (I know it may sound a bit philosophical), exposing the knowledge about PFMs to MCSubtargetInfo is not ideal. Ideally, the MCSubtargetInfo interface should be small and "abstract enough" to allow the description of different subprocessors for different targets. Anything perf related, should be described by other modules (i.e. in a separate class; alternatively, perf-related knowledge should be described by scheduling models).

What if instead we make PFM descriptors a customizable property tablegen class SchedMachineModel? Something that defaults to an empty set of descriptors.

On X86, knowledge about PFM counters of a vendor/family could be moved to a separate .td file (similarly to what you already do with lib/Target/X86/X86PfmCounters.td).
When a new model is created, people can either use `let` expressions to override the PFM set, or - alternatively - derive from a IntelSchedMachineModel class (vic. AMDSchedMachineModel) that sets some common "defaults".
Alternatively, we could introduce the concept of target vendor/processor family, if that helps mapping models to "default sets of" PFM counters.
As long as people are allowed to customize that set (either by using `let` expressions, or by overriding fiels using a a tablegen derived class), then it should be okay.

Not sure if it makes sense.

The bottom line is: I think we should try to keep the concept of PFMs separate from MCSubtargetInfo  as much as possible.
That being said, I don't have a too strong opinion on this; if other devs think that I am wrong on this, then fine. I don't want to block the development on this.

I hope this helps.
Andrea


Repository:
  rL LLVM

https://reviews.llvm.org/D52932