The pfm counters are now in the MCSubtargetInfo rather than the
MCSchedModel (PR39165).
This also compresses the pfm counter tables (PR37068).
Differential D52932
[MCSched] Bind PFM Counters to the CPUs instead of the SchedModel. courbet on Oct 5 2018, 5:42 AM. Authored by
Details The pfm counters are now in the MCSubtargetInfo rather than the This also compresses the pfm counter tables (PR37068).
Diff Detail
Event Timeline
Comment Actions @gchatelet a bunch of your comments are on old code touched by unwanted reformatting. I ignored these since I have reverted the formatted changes.
Comment Actions Thanks
Comment Actions add missing BtVer2PfmCounters to btver2.
Comment Actions I’m not at work today, but I’d like a bit of time to review this patch. I saw that it has already been accepted. However I am not entirely sure if I like the idea of adding another field to MCSubtargetInfo. I need to think about this. Comment Actions 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. 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). 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. I hope this helps. Comment Actions Hi Andrea, thanks for the comments. I think there are two independent points that we're discussing here: Regarding (1): Regarding (2), I'm not sure I understand extactly what you're suggesting. Are you suggesting I do something like: let PfmCounters = HaswellPfmCounters in { def : HaswellProc<"haswell">; def : HaswellProc<"core-avx2">; // Legacy alias. } Or more like: // In X86.td: def HaswellProcessorModel : HaswellProc<"haswell">; def CoreAvx2ProcessorModel : HaswellProc<"core-avx2">; // Legacy alias. // In X86PfmCounters.td: def : ProcPfmCounters { let CycleCounter = UnhaltedCoreCyclesPfmCounter; let UopsCounter = UopsIssuedPfmCounter; let IssueCounters = [ PfmIssueCounter<"HWPort0", "uops_dispatched_port:port_0">, PfmIssueCounter<"HWPort1", "uops_dispatched_port:port_1">, PfmIssueCounter<"HWPort2", "uops_dispatched_port:port_2">, PfmIssueCounter<"HWPort3", "uops_dispatched_port:port_3">, PfmIssueCounter<"HWPort4", "uops_dispatched_port:port_4">, PfmIssueCounter<"HWPort5", "uops_dispatched_port:port_5">, PfmIssueCounter<"HWPort6", "uops_dispatched_port:port_6">, PfmIssueCounter<"HWPort7", "uops_dispatched_port:port_7"> ]; let ProcModels = [HaswellProcessorModel, CoreAvx2ProcessorModel] } Comment Actions Thanks for the description Clement, Sorry. I didn't realize this patch was trying to fix that issue. I still think it is a preferrable solution to not touch MCSubtargetInfo. That being said, I don't have a strong opinion; other reviewers are definitely more knowledgeable than me on exegesis.. Andrea Comment Actions Sorry for late reply! It looks like we need to find a way to still embed this information in MCExtraProcessorInfo but allow for different CPUs PFM mappings. One of the aims for PR39165 was to make it possible for llvm-exegesis to be run on CPUs with declared PFMs but without a model - allowing a report on raw resource usage for an instruction and to help create the model from scratch. @courbet @gchatelet do you think this would still be useful? Is it still an aim of llvm-exegesis to create models from scratch or just report on existing models? Can you easily attach PFM mappings to the GenericModel/GenericPostRAModel? Comment Actions I don't think an MCExtraProcessorInfo should store information for several CPUs, because MCExtraProcessorInfo is in MCSchedModel, which is selected by CPU, so that would be weird. Before this change, we were putting the MCPfmCountersInfo inside MCSchedModel, which implicitly meant that there was a 1:1 mapping between them. This change decouples MCPfmCountersInfo from MCSchedModel. Now the decision we have to make now is whether in TD files (see point (2) in my comment above):
@lebedev.ri used llvm-exegesis to produce the bdverX model. At that point there was no SchedModel, so it still seems desirable to do this. Comment Actions
You say that but we have multiple, very different, CPUs referencing the same model - Knights Landing uses the Haswell Model, the PileDriver model is likely to be used for the entire Bulldozer range, Sandy Bridge model is used as the default cpu - if you run llvm-exegesis in any of these cases apart for the 'main' cpu case it will just crash.
(C) As the PFMs are only used by llvm-exegesis (and are likely to only every be) the PFM mappings should be moved entirely into llvm-exegesis code.
IIRC @lebedev.ri had to create a copy of the btver2 model and then slowly iterate on it until it matched the bdver arch. Comment Actions That's exactly my point: Because we have very different CPUs referencing the same SchedModel, we should not store stuff that's going to vary from CPU to CPU (e.g. PfmCounters) inside the MCSchedModel.
This is choice (B) here, plus choice (1) in the comment I mentioned above, quoting myself: "That being said, I'm not opposed to moving the table of CPU->MCPfmCountersInfo outside of the MCSubtargetInfo. One possible approach is to create a new PfmEmitter tablegen backend and PfmCounters library. If people think that's reasonable I can do that easily." So it seems we have a plan now :)
Comment Actions Move pfm counters to ExegesisTarget. There is now a separate Exegesis TableGen backend. Comment Actions Thanks Clement! I really like this approach. Overall, I think it is a much better design than the original one, since it avoid polluting MCSubtargetInfo. Cheers Comment Actions @courbet: Your commit seems to cause build issues: http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/838 Thanks Comment Actions Simon fixed this issue at r345252 -Andrea
|
This list is in alphabetical order. Mind putting this entry in alphabetical order too?