[PATCH] D48691: [llvm-mca] Add a HardwareUnit and Context classes.

[Matt Davis via Phabricator via llvm-commits]

mattd added a comment.

Thanks for the review @courbet!



================
Comment at: tools/llvm-mca/Context.cpp:30
+
+// Construct a simple pipeline for simulating an out-of-order backend.
+std::unique_ptr<Pipeline> Context::createDefaultPipeline(
----------------
courbet wrote:
> The comment is already on the declaration. I think you can remove it here.
Gotcha.  I'll remove it.


================
Comment at: tools/llvm-mca/Context.h:12
+/// This file defines a class for holding ownership of various simulated
+/// hardware components.  A Context also provides utility routines that make
+/// use of its simulated hardware components.
----------------
courbet wrote:
> I don;t see any such "utility routines" for now. Remove ?
I was considering createDefaultPipeline a "utility routine'."   I plan on adding another, similar, utility routine shortly.


================
Comment at: tools/llvm-mca/HardwareUnit.h:25
+  HardwareUnit &operator=(const HardwareUnit &H) = delete;
+  virtual ~HardwareUnit() = default;
+};
----------------
courbet wrote:
> [nit] Let's make this the class anchor.
Good call.  Thanks for the suggestion.  I went ahead and added an 'anchor' method instead of pinning the dtor.  We already added an explicit anchor to HWEventListener.


https://reviews.llvm.org/D48691