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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Matt,

Thanks for the patch. See my comments below:



================
Comment at: tools/llvm-mca/Context.h:11-13
+/// 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.
----------------
We should be consistent with the terminology. I suggest to use `hardware unit` instead of `hardware component`.


================
Comment at: tools/llvm-mca/Context.h:38
+
+  void addHardware(std::unique_ptr<HardwareUnit> H) {
+    Hardware.push_back(std::move(H));
----------------
s/addHardware/addHardwareUnit


================
Comment at: tools/llvm-mca/Context.h:44-48
+  std::unique_ptr<Pipeline> createDefaultPipeline(
+      unsigned DispatchWidth, unsigned RegisterFileSize, unsigned LoadQueueSize,
+      unsigned StoreQueueSize, bool AssumeNoAlias, SourceMgr &Src,
+      InstrBuilder &IB, const llvm::MCSchedModel &SM,
+      const llvm::MCSubtargetInfo &STI, const llvm::MCRegisterInfo &MRI);
----------------
I was wondering whether we should have a `PipelineOptions` struct which encapsulates all the unit options. The factory method `createDefaultPipeline` would take a PipelineOptions as input; that would simplify the argument list.


================
Comment at: tools/llvm-mca/HardwareUnit.h:25
+  HardwareUnit &operator=(const HardwareUnit &H) = delete;
+  virtual ~HardwareUnit() = default;
+};
----------------
mattd wrote:
> 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.
Can we avoid adding method `anchor()`? You should be able to define the destructor in the .cpp file.


================
Comment at: tools/llvm-mca/HardwareUnit.h:11-12
+///
+/// This file defines a base class for describing a simulated hardware
+/// component.  These components are used to construct a simulated backend.
+///
----------------
Hardware units.


================
Comment at: tools/llvm-mca/llvm-mca.cpp:499-502
+    mca::Context Ctx;
+    auto P = Ctx.createDefaultPipeline(Width, RegisterFileSize, LoadQueueSize,
+                                       StoreQueueSize, AssumeNoAlias, S, IB, SM,
+                                       *STI, *MRI);
----------------
I think the Context should be created only once, and then reused by every CodeSnippet. The Context should be the same for every CodeSnippet.


https://reviews.llvm.org/D48691