[PATCH] D44839: [llvm-mca] Add flag -instruction-tables to print the theoretical resource pressure distribution for instructions (PR36874)

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb marked 2 inline comments as done.
andreadb added a comment.

Thanks Clement,

I am going to address all your review comments before committing the patch.

Cheers,
Andrea



================
Comment at: tools/llvm-mca/InstrBuilder.h:54
   const InstrDesc &getOrCreateInstrDesc(const llvm::MCInst &MCI);
+  llvm::ArrayRef<uint64_t> getProcResourceMasks() const {
+    return ProcResourceMasks;
----------------
courbet wrote:
> I don't remember whether I saw a comment with the definition of the masks somewhere or if we just discussed that together. Can you add a comment here, either pointing to where the notion of mask is defined, or the definition (something like "`ProcResourceMasks[I] & J` is true if resource `I` has a subresource `J`".
These are the processor resource masks computed by function mca::computeProcResourceMasks in Support.h.
In Support.h there is a description of how the mask works.
I will add a  comment explaining this.


================
Comment at: tools/llvm-mca/InstructionTables.cpp:60
+      // Uniformly distribute Cycles across all of the units.
+      for (unsigned I1 = 0, E1 = NumUnits; I1 < E1; ++I1) {
+        unsigned SubUnitIdx = ProcResource.SubUnitsIdxBegin[I1];
----------------
courbet wrote:
> Getting rid of E1 and using NumUnits instead would make everything clearer.
I will do it.


================
Comment at: tools/llvm-mca/InstructionTables.h:11
+///
+/// This file implements a custom driver to generate instruction tables.
+///
----------------
courbet wrote:
> Please add a comment to define what "instruction tables" are, maybe just:
> "see the -instruction-tables command-line flag in docs/CommandGuide/llvm-mca.rst"
Right. I will add that reference.


https://reviews.llvm.org/D44839