[PATCH] D54142: [ARM] Cortex-M4 schedule

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: lib/Target/ARM/ARM.td:980
 
-def : ProcessorModel<"cortex-m3", CortexM3Model,        [ARMv7m,
+def : ProcessorModel<"cortex-m3",   CortexM4Model,      [ARMv7m,
                                                          ProcM3,
----------------
javed.absar wrote:
> Would it be better to rename CortexM4Model as something more generic e.g. CortexMEfficientModel if it really benefits more than just M4.
I think M4 makes sense. It does use the latencies from that core, and the others are just close-enough that it still makes sense to use it. In the same way that we still use the A57 schedule for the A72. I don't believe this model would be used for v6m targets like the M0/M23, or for larger things like the M7.


================
Comment at: lib/Target/ARM/ARMScheduleM4.td:24
+
+let SchedModel = CortexM4Model in {
+
----------------
javed.absar wrote:
> wrong indentation for let
I think this is the same as for other schedules, such as the R52 or the AArch64's A53. Unless I am missing what you mean? I will move the M4Unit above it though.


================
Comment at: lib/Target/ARM/ARMScheduleM4.td:26
+
+def M4Unit   : ProcResource<1> { let BufferSize = 0; }
+
----------------
javed.absar wrote:
> Is this (BufferSize = 0) necessary here for in-order (given, MicroOpBufferSize = 0)?
It seems that if BufferSize is left off, it gets a default value of -1 in the generated ARMGenSubtargetInfo file. I added a comment.


https://reviews.llvm.org/D54142