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

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb requested changes to this revision.
andreadb added a comment.
This revision now requires changes to proceed.

Hi Matt,

Thanks for working on this.
There are a couple of changes that I don't quite understand. Unless I am missing something, I think the patch could have been a lot simpler. See my comments below.

Thanks,
-Andrea



================
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;
----------------
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.


================
Comment at: tools/llvm-mca/InstructionTables.h:32
   InstrBuilder &IB;
-  SourceMgr &S;
   llvm::SmallVector<std::unique_ptr<View>, 8> Views;
+  llvm::SmallVector<std::pair<ResourceRef, double>, 4> UsedResources;
----------------
This is not needed.


================
Comment at: tools/llvm-mca/InstructionTables.h:40
   void addView(std::unique_ptr<View> V) {
+    addListener(V.get());
     Views.emplace_back(std::move(V));
----------------
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.


================
Comment at: tools/llvm-mca/llvm-mca.cpp:493-507
+      auto IT = llvm::make_unique<mca::InstructionTables>(SM, IB);
       if (PrintInstructionInfoView) {
-        IT.addView(
+        IT->addView(
             llvm::make_unique<mca::InstructionInfoView>(*STI, *MCII, S, *IP));
       }
+      IT->addView(llvm::make_unique<mca::ResourcePressureView>(*STI, *IP, S));
+
----------------
Unless I am missing something, I think you should simply use a PipelinePrinter. I don't think that any follow-on patch is actually needed here. If you use a PipelinePrinter, then it becomes the owner of all the views.


================
Comment at: tools/llvm-mca/llvm-mca.cpp:494-499
       if (PrintInstructionInfoView) {
-        IT.addView(
+        IT->addView(
             llvm::make_unique<mca::InstructionInfoView>(*STI, *MCII, S, *IP));
       }
+      IT->addView(llvm::make_unique<mca::ResourcePressureView>(*STI, *IP, S));
+
----------------
Why this? You should be able to use a PipelinePrinter, and add View objects directly to the printer.
The printer would then subscribe views to each stage of the pipeline.


https://reviews.llvm.org/D49329