[PATCH] D47246: [llvm-mca] Introduce the ExecuteStage (was originally the Scheduler class).

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Matt

thanks for the patch. Sorry for the late review; it took me a while to go through all of its parts (I was not expecting so many changes). 
I have a few nits (see my comments below):



================
Comment at: tools/llvm-mca/ExecuteStage.cpp:30
+
+void ExecuteStage::preExecute(const InstRef &Unused) {
+  SmallVector<ResourceRef, 8> ResourcesFreed;
----------------
Something like a small paragraph with a brief overview of the sequence of steps performed by this method would really help. I know that it was missing in the original implementation. However, I think it really helps a lot if we add a small description here.

Some steps could be moved to a separate function. That might help with making the code more readable (if we choose good names for functions).


================
Comment at: tools/llvm-mca/ExecuteStage.cpp:78
+#ifndef NDEBUG
+  HWS.sanityCheck(IR);
+#endif
----------------
Please add a small comment here.


================
Comment at: tools/llvm-mca/HWEventListener.h:106-107
+    LastGenericEvent,
+    // No stall hazard has occurred.
+    NoStall
   };
----------------
See my other comment.


================
Comment at: tools/llvm-mca/Scheduler.cpp:235-236
 
-bool Scheduler::canBeDispatched(const InstRef &IR) const {
+HWStallEvent::GenericEventType
+Scheduler::canBeDispatched(const InstRef &IR) const {
   HWStallEvent::GenericEventType Type = HWStallEvent::Invalid;
----------------
Could you change this method so that it returns a bool? I don't particularly like the fact that , as a consequence of this change, we now need a new value in the GenericEventType enum.
You can make the enum an in/out argument to this method. If `canBeDispatched` returns false, then the `HWStallEvent::GenericEventType` contains the reason why dispatch failed.


================
Comment at: tools/llvm-mca/Scheduler.h:462
 
-  void cycleEvent();
+  /// Update the resources managed by the scheduler.
+  void reclaimSimulatedResources(llvm::SmallVectorImpl<ResourceRef> &Freed);
----------------
Emphasize that this method is meant to be called when a new cycle starts.
Also, add a small sentence to better describe the semantic of this method.


================
Comment at: tools/llvm-mca/Scheduler.h:497-499
 #endif
+
+#ifndef NDEBUG
----------------
You can reuse the previous NDEBUG block. I think you should remove the `#endif` at line 497 instead of opening a new block.


https://reviews.llvm.org/D47246