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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

Thank you, looks good in general!
Some very contrived nits:



================
Comment at: tools/llvm-mca/Views/SchedulerStatistics.cpp:35-45
+// FIXME: This implementation works under the assumption that load/store queue
+// entries are reserved at 'instruction dispatched' stage, and released at
+// 'instruction executed' stage. This currently matches the behavior of LSUnit.
+//
+// The current design minimizes the number of events generated by the
+// Dispatch/Execute stages, at the cost of doing extra bookkeeping in method
+// `onEvent`. However, it introduces a subtle dependency between this view and
----------------
Is there a tracking bug for this? Could you file one?
And reference it here in the comment please.


================
Comment at: tools/llvm-mca/include/HardwareUnits/LSUnit.h:130-139
+  // FIXME: On some processors, load/store operations are split into multiple
+  // uOps. For example, X86 AMD Jaguar natively supports 128-bit data types, but
+  // not 256-bit data types. So, a 256-bit load is effectively split into two
+  // 128-bit loads, and each split load consumes one 'LoadQueue' entry. For
+  // simplicity, this class optimistically assumes that a load instruction only
+  // consumes one entry in the LoadQueue.  Similarly, store instructions only
+  // consume a single entry in the StoreQueue.
----------------
Is there a bug tracking this?


================
Comment at: tools/llvm-mca/include/HardwareUnits/LSUnit.h:196-198
+  // FIXME: For simplicity, we optimistically assume a similar behavior for
+  // store instructions.  In practice, store operation don't tend to leave the
+  // store queue until they reach the 'Retired' stage.
----------------
Is there a bug tracking this?


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

https://reviews.llvm.org/D54957