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

[Matt Davis via Phabricator via llvm-commits]

mattd 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:
> 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&.
> 
That's cool with me.  We will definitely need to make SourceMgr a required input to createDefaultPipeline.  I made InstrBuilder a member of the PipelineOptions structl; we only ever construct that item once in llvm-mca, outside of the region generation loop. 


================
Comment at: tools/llvm-mca/Context.h:33
+/// the pre-built "default" out-of-order pipeline.
+struct PipelineOptions {
+  PipelineOptions(unsigned DW, unsigned RFS, unsigned LQS, unsigned SQS,
----------------
I not sure if we want to have the PipelineOptions struct nested within the Context class or not.   I'm unbiased to either decision, and felt that exposing it as part of the  root mca namespace isn't a bad decision.


https://reviews.llvm.org/D48691