[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 3 inline comments as done.
andreadb added a comment.

In D54957#1311632 <https://reviews.llvm.org/D54957#1311632>, @lebedev.ri wrote:

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


Thanks for the review Roman!



================
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
----------------
lebedev.ri wrote:
> Is there a tracking bug for this? Could you file one?
> And reference it here in the comment please.
Sure. I am going to create a bug for it.


================
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.
----------------
lebedev.ri wrote:
> Is there a bug tracking this?
I definitely plan to raise a bug for the usage of field `LoadLatency`.

I am not entirely sure about whether there is a good/reasonable way to address the last FIXME paragraph. If you want, I can raise a low-priority investigation bug.


================
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.
----------------
lebedev.ri wrote:
> Is there a bug tracking this?
There isn't. I am not sure if we want to revisit this in future. I suspect, itt require a bit more investigation. That being said, if you want, I can raise an investigation bugzilla for it.


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

https://reviews.llvm.org/D54957