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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb marked 6 inline comments as done.
andreadb added inline comments.


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


================
Comment at: tools/llvm-mca/Views/SchedulerStatistics.cpp:49
     ++NumIssued;
+  else if (Event.Type == HWInstructionEvent::Dispatched) {
+    const Instruction &Inst = *Event.IR.getInstruction();
----------------
mattd wrote:
> 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?
On targets that don't specify a load/store queue, resource usage is normally updated by `onReservedBuffers` and `onReleasedBuffers`.
Basically, this patch is not a visibile change for processors that don't declare a load/store queue.


================
Comment at: tools/llvm-mca/Views/SchedulerStatistics.cpp:87
+  for (const unsigned Buffer : Buffers) {
+    if (Buffer == LQResourceID || Buffer == SQResourceID)
+      continue;
----------------
mattd wrote:
> 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.`
> 
> 
ID zero is for the invalid resource. So, it cannot be a buffered resource.


================
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.
----------------
mattd wrote:
> nit:  Remove the wild '.' character in the middle of the sentence.
Will do.


================
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
----------------
mattd wrote:
> 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.
No documentation in particular... Processor vendors often describe it in their official documents. Maybe it is my English... if it is confusing, I can remove it.


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


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

https://reviews.llvm.org/D54957