[PATCH] D28152: Cortex-A57 scheduling model for ARM backend (AArch32)

[Andrew Zhogin via Phabricator via llvm-commits]

andrew.zhogin marked 9 inline comments as done.
andrew.zhogin added inline comments.


================
Comment at: lib/Target/ARM/ARMScheduleA57.td:1214
+// ASIMD reverse
+def : InstRW<[A57Write_3cyc_1V], (instregex "VREV16", "VREV32", "VREV64")>;
+
----------------
javed.absar wrote:
> You may want to combine VREV* VSWP*  VTBL .. into one instregex comma separated def, to make definitions shorter.
> Similarly in other places too, where possible without compromising clarity
Well, I tried to keep structure from the documentation. 1 row from table = 1 InstRW def. To be easy to verify.


================
Comment at: lib/Target/ARM/ARMScheduleA57WriteRes.td:754
+
+def A57Write_6cyc_6S_4V : SchedWriteRes<[A57UnitS, A57UnitS, A57UnitS,
+                                         A57UnitS, A57UnitS, A57UnitS,
----------------
javed.absar wrote:
> Something seems amiss as its defined earlier there is only 1 processor-unit of type A57UnitS. 
Unused scheduling classes deleted (legacy from AArch64).


================
Comment at: test/CodeGen/ARM/cortex-a57-misched-alu.ll:30
+
+; CHECK:      ** ScheduleDAGMILive::schedule picking next node
+; Skipping COPY
----------------
javed.absar wrote:
> This part below is not required I think as you have already checked the latencies here against the model.
I did it to check scheduling units (A57UnitI or A57UnitM). Not just latency.


https://reviews.llvm.org/D28152