[PATCH] D45360: [MC][TableGen] Add optional libpfm counter names for ProcResUnits.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Clement,

I prefer a more flexible design where perf events can be defined for all resources, and not just for ProcResourceUnits.
In future, we may want to add profiling information to resources that are not just pipeline ports. For example, we could add profiling support for hardware scheduler resources. Essentially groups should be considered too.

I also prefer that we move all the "optional" information into MCExtraProcessorInfo. I don't like the idea of adding an extra pointer to every single resource defined by a processor (especially if the processor doesn't care about describing perf counters).

This is just an idea for an alternative design:
You could define a separate tablegen class to describe the mapping of resources to perf events. Something like this:

  class PFMCounter;
  class DispatchPort0Counter : PFMCounter;
  class DispatchPort1Counter : PFMCounter;
   /*etc.,  ... enumerate all counters here ... */
  
  class PerfCounter<ProcResourceKind Kind, list<PFMCounter> Counters> {
    ProcResourceKind Resource = Kind;
    list<PFMCounter> Counters;  // A list of counter IDs.
  }

Here, PFMCounters would be used to construct a (subtarget specific) enum type, where each enum value is a reference to a PMC string name.
The generic "unhalted_core_cycles" event would get an enum ID too.

This is just an idea for an alternative design. Regardless of whether you end up adopting this solution or not, I believe that it is cleaner if we move anything which is optional into MCExtraProcessorInfo.

Just my opinion.
-Andrea



================
Comment at: include/llvm/MC/MCSchedule.h:252-254
+  // An optional name of a performance counter that can be used to measure
+  // cycles.
+  const char *PfmCycleCounter;
----------------
I think that this should be moved to the MCExtraProcessorInfo data structure.


================
Comment at: utils/TableGen/SubtargetEmitter.cpp:702
      << "[] = {\n"
-     << "  {\"InvalidUnit\", 0, 0, 0, 0},\n";
+     << "  {\"InvalidUnit\", 0, 0, 0, 0, nullptr},\n";
 
----------------
Can this be moved to `MCExtraProcessorInfo`?

I am of the opinion that this information is optional, and it shouldn't be included by default in MCProcResourceDesc. See my other comments.


Repository:
  rL LLVM

https://reviews.llvm.org/D45360