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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb accepted this revision.
andreadb added a comment.
This revision is now accepted and ready to land.

LGTM if you address the comments below.

Thanks.



================
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,
----------------
mattd wrote:
> 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.
I think it is fine either way.


================
Comment at: tools/llvm-mca/Context.h:46-48
+  InstrBuilder &IB;
+  const llvm::MCSubtargetInfo &STI;
+  const llvm::MCRegisterInfo &MRI;
----------------
These last three fields are not really "options".
STI and MRI should never change, and those can be safely added to the context, and initialized at construction time.
The InstrBuilder can be passed in input to the call to `createDefaultPipeline()`.
We can change this design in future if we see that there are better designs.


https://reviews.llvm.org/D48691