[PATCH] D49329: [llvm-mca] Turn InstructionTables into a Stage.

[Matt Davis via Phabricator via llvm-commits]

mattd marked an inline comment as done.
mattd added inline comments.


================
Comment at: tools/llvm-mca/InstructionTables.cpp:35-58
+    const unsigned Index = std::distance(
+        Masks.begin(), std::find(Masks.begin(), Masks.end(), Resource.first));
+    const MCProcResourceDesc &ProcResource = *SM.getProcResource(Index);
+    const unsigned NumUnits = ProcResource.NumUnits;
+    if (!ProcResource.SubUnitsIdxBegin) {
+      // The number of cycles consumed by each unit.
+      Cycles /= NumUnits;
----------------
andreadb wrote:
> We don't tend to add `const` to local variables with primitive types. Although, technically it is not wrong, it needlessly increases the level of verbosity for no good reason (no value in return).
> In several parts of LLVM, `const` is mainly associated with reference/pointer types, mainly to avoid that non-const methods are called on them, and to emphasize that data is not changed through those pointers.
> 
> In this particular case, adding const doesn't make things better imho. I also don't want that we end up doing similar changes to other files of this project. Since this change is not really needed to fix the issue with the patch, I suggest to remove it if you don't mind.
Fair point.  I tend to do this as part of my personal style, indicating that a value will not change, implying that there's no need to hunt down any writes to the variable later on.  But your point is clear, I'll undo it.  


================
Comment at: tools/llvm-mca/InstructionTables.h:40
   void addView(std::unique_ptr<View> V) {
+    addListener(V.get());
     Views.emplace_back(std::move(V));
----------------
andreadb wrote:
> mattd wrote:
> > I'm working on a follow on patch to clean this addListener call up.
> Hang on. 
> You shouldn't have this method at all! Just use a PipelinePrinter to generate the report.
I didn't consider a PipelinePrinter, I kept the InstructionTables  close to its original structure with its original Printer-like representation (addView/printReport/Views).  PipelinePrinter will remove the need for those bits, sorry for that. 

Note that  InstructionTables is now a Stage, with listeners.  We will still need to associate the events generated by the stage to the views.  This means we will need to make a call to addListener to IT.  If we don't do this, then the resource pressure view will not get any data.  


https://reviews.llvm.org/D49329