[PATCH] D54957: [llvm-mca][MC] Add the ability to declare which processor resources model load/store queues (PR36666).

[Matt Davis via Phabricator via llvm-commits]

mattd added a comment.

LGTM, I'll let others weigh in.  For the most part I think this patch is fine, as long as my questions do not point out any bugs.



================
Comment at: include/llvm/Target/TargetSchedule.td:567
+// which describe load/store queues in the LS unit.
+class MemoryQueue<ProcResource ProcResource> {
+  ProcResource QueueDescriptor = ProcResource;
----------------
I'd suggest renaming the parameter something different, perhaps  `<ProcResource PR>`.  Reading `<ProcResource ProcResource>` caused my brain to hiccup a bit.


================
Comment at: tools/llvm-mca/Views/SchedulerStatistics.cpp:49
     ++NumIssued;
+  else if (Event.Type == HWInstructionEvent::Dispatched) {
+    const Instruction &Inst = *Event.IR.getInstruction();
----------------
I'm curious.  What if the target does not have a LoadQueueID or StoreQueueID defined?  It seems like SchedulerStats would not account for the uses in Usage vector in that case.  Should we emit a warning if that is the case, or is it reasonable for SchedulerStats to ignore targets that do not define the Load/Store IDs?


================
Comment at: tools/llvm-mca/Views/SchedulerStatistics.cpp:87
+  for (const unsigned Buffer : Buffers) {
+    if (Buffer == LQResourceID || Buffer == SQResourceID)
+      continue;
----------------
If a target does not define a LQ/SQ resource ID, then SchedulerStats defaults to an ID of '0'.  Is it possible for there to be a valid buffer with a BufferID of 0, or is that some special case?  Our resource Buffer IDs come from an index defined in the processor model: 
  `/// An index to the MCProcResourceDesc entry in the processor model.`




================
Comment at: tools/llvm-mca/include/HardwareUnits/LSUnit.h:108
+  // instruction is eventually removed from the LoadQueue when it reaches
+  // completion stage. That means. the load must be 'executed', and the LS unit
+  // must be able to forward the loaded value on the data path.
----------------
nit:  Remove the wild '.' character in the middle of the sentence.


================
Comment at: tools/llvm-mca/include/HardwareUnits/LSUnit.h:120
+  // However, field `LoadLatency` is often based on the 'load-to-use' latency
+  // from L1D (as reported in the official hardware documentation), and it
+  // normally already accounts for the extra latency of due to the forwarding
----------------
It feels weird to mention 'official hardware documentation' here, when the LSUnit should be somewhat target agnostic.  Perhaps clarify what hardware documentation you are referring to.


================
Comment at: tools/llvm-mca/lib/HardwareUnits/LSUnit.cpp:29
+  if (SM.hasExtraProcessorInfo()) {
+    const MCExtraProcessorInfo &MEP = SM.getExtraProcessorInfo();
+    if (!LQ_Size && MEP.LoadQueueID) {
----------------
nit:  In SchedulerStatistics.cpp you named the instance of a MCExtraProcessorInfo as 'EPI' and here you are calling it 'MEP'. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54957/new/

https://reviews.llvm.org/D54957