[PATCH] D47374: [RFC][patch 3/3] Add support for variant scheduling classes in llvm-mca.

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: lib/Target/X86/X86ScheduleBtVer2.td:550
+////////////////////////////////////////////////////////////////////////////////
+//  MCInstPredicate definitions used by variant scheduling classes.
+////////////////////////////////////////////////////////////////////////////////
----------------
Add a TODO saying this may go into X86Schedule.td in the future?


================
Comment at: lib/Target/X86/X86ScheduleBtVer2.td:575
+// Reference: Section 10.8 of the "Software Optimization Guide for AMD Family
+// 15h Processors".
+
----------------
Reference Agner's microarchitecture doc as well - he explicity says this works for Jaguar - AMD 16h SOG unfortunately misses it and just having a reference to a different cpu is confusing.


================
Comment at: test/CodeGen/X86/sse-schedule.ll:6084
 ; BTVER2-SSE:       # %bb.0:
-; BTVER2-SSE-NEXT:    xorps %xmm1, %xmm0 # sched: [1:0.50]
+; BTVER2-SSE-NEXT:    xorps %xmm1, %xmm0 # sched: [?:?]
 ; BTVER2-SSE-NEXT:    xorps (%rdi), %xmm0 # sched: [6:1.00]
----------------
This is unfortunate - ideally we'd keep [1:0.50] here and zero idioms would get [0:0.50]


================
Comment at: test/tools/llvm-mca/X86/BtVer2/zero-idioms.s:32
+# CHECK-NEXT:  1      0      -                          pxor	%xmm2, %xmm2
+# CHECK-NEXT:  1      0      -                          vpxor	%xmm3, %xmm3, %xmm3
+
----------------
Should the RThroughput still be limited to by issue width?


================
Comment at: tools/llvm-mca/InstrBuilder.cpp:385
+    SchedClassID =
+        STI.resolveVariantSchedClass(SchedClassID, &MCI, SM.getProcessorID());
+  }
----------------
Drop braces - also is it worth pulling out SM.getProcessorID()? It might get this down to a single line for tidiness.


https://reviews.llvm.org/D47374