[PATCH] D52779: AMD BdVer2 (Piledriver) Initial Scheduler model

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Roman,

Thanks for contributing this model! I really enjoyed reading your patch.

I only have a few minor nits (mostly style comments). But, overall, from my point of view, it looks very good!

Thanks
-Andrea



================
Comment at: lib/Target/X86/X86ScheduleBdVer2.td:298-300
+// Nops don't have dependencies, so there's no actual latency, but we set this
+// to '1' to tell the scheduler that the nop uses an ALU slot for a cycle.
+def : WriteRes<WriteNop, [PdEX01]> { let Latency = 1; }
----------------
Latency is set to 1 by default.
Is it because you wanted to emphasize that aspect?
Otherwise, I think you can just write:

```
def : WriteRes<WriteNop, [PdEX01]>;
```


================
Comment at: lib/Target/X86/X86ScheduleBdVer2.td:321-323
+def PdWriteXADD: SchedWriteRes<[PdEX1]> {
+  let Latency = 1;
+}
----------------
Same.


================
Comment at: lib/Target/X86/X86ScheduleBdVer2.td:336
+defm : PdWriteResExPair<WriteIMul64Reg, [PdEX1, PdMul],          6,  [1, 4]>;
+defm : PdWriteRes<WriteIMulH,          [PdEX1],        /*???*/ 10,  [4]>;
+
----------------
Why the question marks?


================
Comment at: lib/Target/X86/X86ScheduleBdVer2.td:382
+
+def  : WriteRes<WriteSETCC,           [PdEX01]> { let Latency = 1; } // Setcc.
+def  : WriteRes<WriteSETCCStore,      [PdEX01, PdStore]>;
----------------
```
def : WriteRes<WriteSETCC, [PdEX01]>;
```


================
Comment at: lib/Target/X86/X86ScheduleBdVer2.td:551
+defm : PdWriteRes<WriteFLoadY,             [PdLoad, PdFPU01, PdFPFMA], 5, [], 2>;
+// defm : X86WriteResPairUnsupported<WriteFLoadZ>;
+
----------------
I am a bit confused. Why do you need this comment? WriteFLoadZ is not even a thing...
I noticed that you do the same in various other places (for other non-existent writes).
I suggest to remove these comments.


================
Comment at: lib/Target/X86/X86ScheduleBdVer2.td:1067
+
+// VXORPSYrr, VXORPDYrr, VANDNPSYrr, VANDNPDYrr "zero-idioms" have latency of 1.
+
----------------
Do you plan to add these too?
I noticed that you have marked those as dep-breaking. However, if I read correctly, those still map to WriteFLogicY, which declares 2 resource cycles. Presumably these zero-idioms should only consume 1 resource cycle (to execute the zero-move to the upper half of YMM).


Repository:
  rL LLVM

https://reviews.llvm.org/D52779