[PATCH] D63543: [MCA][Bottleneck Analysis] Teach how to compute a critical sequence of instructions based on the simulation.

[Andrea Di Biagio via Phabricator via llvm-commits]

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


================
Comment at: test/tools/llvm-mca/X86/SkylakeClient/bottleneck-analysis.s:59
+# CHECK-NEXT:  |      2.    vmulps	-24(%rsp), %xmm7, %xmm8
+# CHECK-NEXT:  +----> 3.    vpermilps	$170, %xmm0, %xmm6                ## REGISTER dependency:  %xmm0
+# CHECK-NEXT:  |      4.    vpermilps	$85, %xmm0, %xmm5
----------------
RKSimon wrote:
> (very pedantic) Fix the Dependency Information column indentation to be consistent? 
Very strange.

I suspect it probably has to do with the fact that update_mca_check script might have replaced tabs with spaces.
MCInstPrinter inserts tabs between opcode and operand list. Those don't seem to be preserved by the check script, so you end up with that odd check line.
When printing the dependency information, I already "pad-to-column". So, to me that must have to do with the script that generates check lines.
(It appears correctly aligned on my terminal).


================
Comment at: tools/llvm-mca/Views/BottleneckAnalysis.cpp:210
+    SmallVectorImpl<const DependencyEdge *> &Seq) const {
+  const auto It = std::max_element(
+      Nodes.begin(), Nodes.end(),
----------------
RKSimon wrote:
> Comments?
I will add a description of the algorithm here.


================
Comment at: tools/llvm-mca/Views/BottleneckAnalysis.h:139
 
-  // Pair of vertices connected by this edge.
+  // Pair of vertices which defines this edge.
   unsigned FromIID;
----------------
RKSimon wrote:
> A lot of this is independent of the patch - precommit cleanup?
Sorry. The data dependency graph was pre-committed to "simplify" this patch.
In retrospect I am not sure if that was a wise choice. So, apologies in case.

I guess, the least that I can do is to add more code comments. Let me know...


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63543/new/

https://reviews.llvm.org/D63543