[PATCH] D45259: [MC][Tablegen] Allow models to describe the retire control unit for llvm-mca.

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added inline comments.


================
Comment at: tools/llvm-mca/Dispatch.cpp:262
+      AvailableSlots = EPI.ReorderBufferSize;
+    if (!MaxRetirePerCycle)
+      MaxRetirePerCycle = EPI.MaxRetirePerCycle;
----------------
courbet wrote:
> Can we get rid of the MaxRetirePerCycle ctor parameter ? Is there any reason why the user would override this ?
The original idea was to let users override the retire-per-cycle.
However, with this patch, the max-retire-per-cycle can now be set via tablegen.

That being said, most processors don't provide a max-retire-per-cycle. This patch would only fix the BtVer2 model.
So, I am tempted to keep the flag for now (although, I don't have a strong opinion about it). Alternatively, I can commit a separate patch that removes the flag, and the rebase this patch. What do you think?



================
Comment at: tools/llvm-mca/Dispatch.h:194
 
+  void initialize(const llvm::MCSchedModel &SM);
+
----------------
courbet wrote:
> Any reason for this to be in a separate function ? What about moving the ctor definition to the cpp file and inlining the code ?
> Else when reading the ctor definition here it's not obvious that AvailableSlots might be overridden in initialize().
No reason. I will move the constructor to the cpp file.


https://reviews.llvm.org/D45259