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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
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; }
----------------
andreadb wrote:
> 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]>;
> ```
No, not really intentionally.


================
Comment at: lib/Target/X86/X86ScheduleBdVer2.td:336
+defm : PdWriteResExPair<WriteIMul64Reg, [PdEX1, PdMul],          6,  [1, 4]>;
+defm : PdWriteRes<WriteIMulH,          [PdEX1],        /*???*/ 10,  [4]>;
+
----------------
andreadb wrote:
> Why the question marks?
Because i wasn't able to measure this, and up until now, i didn't trace it back to the definition.
Now i see it's BMI2 instruction, so i'll mark this as unsupported.

Also, it would be really good to have some automatic (a warning) to be notified of cases like this.
I do realize it won't really work for default generic sched models used for many different CPUs.


================
Comment at: lib/Target/X86/X86ScheduleBdVer2.td:551
+defm : PdWriteRes<WriteFLoadY,             [PdLoad, PdFPU01, PdFPFMA], 5, [], 2>;
+// defm : X86WriteResPairUnsupported<WriteFLoadZ>;
+
----------------
andreadb wrote:
> 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.
There are remnants from when i was looking which sched classes were somehow not listed here; dropped.


================
Comment at: lib/Target/X86/X86ScheduleBdVer2.td:1067
+
+// VXORPSYrr, VXORPDYrr, VANDNPSYrr, VANDNPDYrr "zero-idioms" have latency of 1.
+
----------------
andreadb wrote:
> 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).
Resource cycles is something i haven't touched at all.. Like completely at all.
I do not even really understand how they are calculated.

I'm not fully sure what/how this should be, so i'd leave this as-is for now..


Repository:
  rL LLVM

https://reviews.llvm.org/D52779