[PATCH] D52779: AMD BdVer2 (Piledriver) Initial Scheduler model

Wed Oct 3 02:09:19 PDT 2018

courbet added inline comments.

================
Comment at: lib/Target/X86/X86.td:1025
 // Piledriver
-def : Proc<"bdver2", [
+def : ProcessorModel<"bdver2", BdVer2Model, [
   FeatureX87,
----------------
RKSimon wrote:
> lebedev.ri wrote:
> > craig.topper wrote:
> > > Should we apply this to bdver3/4 as well? Are they similar?
> > Hmm. It would be ok to apply this to bdver1.
> > 
> > But i'm not sure about steamroller/excavator.
> > Those are more different, have loop buffer, 3 FPU pipes instead of 4, etc.
> I'd expect it to help all bdver targets - but as @lebedev.ri said the arch is slightly different on bdver3/bdver4 - for instance the exegesis pfms for the pipe resources won't work correctly (and may crash).
> 
> We have the same problem with the generic x86_64 cpu which expects SNB pfms - @courbet how tricky would it be to attach the pfm counters to a set of CPUs instead of the scheduler model? This might even help creation of new models if we can get pfm (cycles/uops) data from cpus without a model.
> how tricky would it be to attach the pfm counters to a set of CPUs instead of the scheduler model

The main reason why I added pfm counters to the sched model is that we want analysis to be able to match counters to resource names.
e.g. `PfmIssueCounter<SBPort0, ["uops_dispatched_port:port_0"]>;` binds the counter to SBPort0, and the TD resolver automatically checks that SBPort0 exists in the correct sched model.

What we could do is: 

  - move the `PfmCountersInfo` out of `MCSubtargetInfo`->`MCSchedModel`->`MCExtraProcessorInfo` and put it in `MCSubtargetInfo` directly.
  - make `SchedModel` optional and take the resource  as a string in `PfmIssueCounter` ( `PfmIssueCounter<"SBPort0", ["uops_dispatched_port:port_0"]>;`), and do the resolving/checking in the SubtargetEmitter **only if** the subtarget has a sched model.

Sounds reasonable ?

Repository:
  rL LLVM

https://reviews.llvm.org/D52779