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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
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);
----------------
andreadb wrote:
> 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.
I still think that - for simplicity and readability - we should wrap all those flags into a PipelineOptions struct.

We don't need to pass a MCSchedModel to createDefaultPipeline because we can always query STI to obtain it.

Even better, we can have that MCSubtargetInfo and MCRegisterInfo are passed in input to the constructor of Context, and used internally by its "utility" methods.

If we do all of this, then the `createDefaultPipeline()` would simply take a `PipelineOptions`, a SourceMgr& and a InstrBuilder&.



================
Comment at: tools/llvm-mca/Context.h:12-13
+/// This file defines a class for holding ownership of various simulated
+/// hardware units.  A Context also provides utility routines that make
+/// use of its simulated hardware units.
+///
----------------
Maybe be more specific here, and say that it provides a method to instantiate a "default" dispatch/execute/retire pipeline.
We can always update this comment in future.


https://reviews.llvm.org/D48691