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

Thu Apr 5 06:32:42 PDT 2018

andreadb marked 3 inline comments as done.
andreadb added inline comments.

================
Comment at: tools/llvm-mca/Dispatch.cpp:262
+      AvailableSlots = EPI.ReorderBufferSize;
+    if (!MaxRetirePerCycle)
+      MaxRetirePerCycle = EPI.MaxRetirePerCycle;
----------------
andreadb wrote:
> courbet wrote:
> > andreadb wrote:
> > > 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?
> > > 
> > I'm pretty sure that I would stick to tablegen-specified models as a user. And if you're developing you own processor, I hope that you have your own tools :) So the flag does not sound that useful to me given that it makes a bit harder to understand this code.
> > I don't have a strong opinion though, so feel free to keep the flag if i'ts useful to you.
> That's a good point (especially if you have your own tools).
> It could have been used to experiment/play with the RCU (with SMT processors we might emulate a different retire throughput).
> That being said, the argument is a bit weak. SMT introduces other issues related to the partitioning of resources which cannot be fully tweaked via flags.
> 
> If okay for you, I am going to commit a change that removes the flag and updates the docs in preparation for this patch. Then I rebase this patch.
I went ahead and removed that flag at r329274.

https://reviews.llvm.org/D45259