[PATCH] D39051: F16C inructions scheduling on btver2

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: lib/Target/X86/X86ScheduleBtVer2.td:395
+  let Latency = 3;
+  let ResourceCycles = [1];
+}
----------------
I think 'ResourceCycles = [1];' is the default so you should be able to drop this?


================
Comment at: lib/Target/X86/X86ScheduleBtVer2.td:403
+}
+def : InstRW<[WriteCVTPS2PHSt], (instregex "VCVTPS2PHmr", "VCVTPH2PSrm")>;
+
----------------
Split off the cvtph2ps instructions from cvtps2ph - the load/store cases are definitely different and it makes little sense to keep the rr cases together.


================
Comment at: lib/Target/X86/X86ScheduleBtVer2.td:405
+
+def WriteCVTPS2PHY: SchedWriteRes<[JFPU0, JFPU1]> {
+  let Latency = 6;
----------------
Shouldn't the JFPU1 case be JFPU01? The amd docs say 'STC,FPA|FPM'


================
Comment at: lib/Target/X86/X86ScheduleBtVer2.td:412
+
+def WriteCVTPS2PHYSt: SchedWriteRes<[JFPU0, JFPU1, JLAGU]> {
+  let Latency = 11;
----------------
Shouldn't the JFPU1 case be JFPU01?


================
Comment at: lib/Target/X86/X86ScheduleBtVer2.td:426
+
+def WriteCVTPH2PSYLd: SchedWriteRes<[JFPU1, JLAGU]> {
+  let Latency = 10;
----------------
cvtph2ps is a load, so the JLAGU is the first stage not the last.


================
Comment at: test/CodeGen/X86/f16c-schedule.ll:48
 ; BTVER2:       # BB#0:
-; BTVER2-NEXT:    vcvtph2ps (%rdi), %xmm1 # sched: [8:1.00]
+; BTVER2-NEXT:    vcvtph2ps (%rdi), %xmm1 # sched: [3:1.00]
 ; BTVER2-NEXT:    vcvtph2ps %xmm0, %xmm0 # sched: [3:1.00]
----------------
This should be [8:1.00]


================
Comment at: test/CodeGen/X86/f16c-schedule.ll:106
+; BTVER2-NEXT:    vcvtph2ps (%rdi), %ymm1 # sched: [5:2.00]
+; BTVER2-NEXT:    vcvtph2ps %xmm0, %ymm0 # sched: [5:2.00]
 ; BTVER2-NEXT:    vaddps %ymm0, %ymm1, %ymm0 # sched: [3:2.00]
----------------
These should be [8:2.00] and [3:2.00]?


https://reviews.llvm.org/D39051