[PATCH] D63628: AMD K10 (Barcelona) Initial Scheduler model

[Andrea Di Biagio via Phabricator via llvm-commits]

andreadb added a comment.

Hi Roman,

thanks for the patch. I left some comments below.

Overall, the patch looks good.
It is okay to contribute improvements to zero-idioms/dep-breaking instructions in a later patch.



================
Comment at: lib/Target/X86/X86ScheduleBarcelona.td:24
+  let MicroOpBufferSize = 72; // 24 lines of three macro-ops.
+  let LoopMicroOpBufferSize = -1; // There does not seem to be a loop buffer.
+  let LoadLatency = 3; // The L1 data cache has a 3-cycle load-to-use latency.
----------------
You can safely remove this line.
The default value of LoopMicroOpBufferSize is already -1.


================
Comment at: lib/Target/X86/X86ScheduleBarcelona.td:45-48
+def BnRCU : RetireControlUnit<72, 3>;
+// FIXME: it isn't that simple actually.
+// It's not 72 entries, but more like 24 "entries", each entry tracking
+// up to 3 lanes.
----------------
I think that your definition of RetireControlUnit is fine.
The RCU is definitely 72 entries. It internally tracks macro-ops contributed to each pipeline. The fact that instructions are distributed among three different pipes after Fetch shouldn't really affect the description of the RCU in LLVM (or require future any changes in future). In practice, you can assume a uniform distribution of decoded ops on the three pipelines. Basically, I don't think that you need a FIXME here.



================
Comment at: lib/Target/X86/X86ScheduleBarcelona.td:286
+
+defm : BnWriteRes<WriteZero, [/*No ExePorts*/], 0, [], 0>; // FIXME
+
----------------
FIXME?


================
Comment at: lib/Target/X86/X86ScheduleBarcelona.td:305
+// to '1' to tell the scheduler that the nop uses an ALU slot for a cycle.
+defm : BnWriteResInt<WriteNop, BnInt012, [BnALU012], 0, [1], 1>; // FIXME
+
----------------
Maybe clarify what there is to FIX.
Did you verify that NOPs consume an ALU slot?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63628