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

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/Target/ARM/ARM.td:783
 
-def : ProcNoItin<"cortex-a57",                          [ARMv8a, ProcA57,
+def : ProcessorModel<"cortex-a57-r0px", CortexA57Model, [ARMv8a, ProcA57,
+                                                         FeatureHWDiv,
----------------
andrew.zhogin wrote:
> rengolin wrote:
> > jgreenhalgh wrote:
> > > I'm not sure that splitting the option like this is a good idea, certainly this would create an incompatibility with GCC, which does not recognise -mcpu=cortex-a57-r0px. In GCC -mcpu=cortex-a57 enables scheduling for all the optimized instruction pairs (e.g. MOV/MOVT), and uses the r0p0 latency values for the Advanced SIMD multiply accumulate instructions.
> > > 
> > > In your patch, this extra flag changes the scheduling of vector multiply, multiply accumulate, and the mov/movt instructions. While I can see that this can improve the resulting schedule in some circumstances, my opinion is that fragmenting the option like this is not worth the extra cost of carrying a special option.
> > James' point is valid even if you ignore GCC compatibility. Scheduler models are "best effort" at best, so splitting it like this makes no sense. If there are multiple revisions of the same core, then pick the most common and optimise for it.
> > 
> > The only case where you can split like that is for recognised new sub-architectures, like `Swift`, `Kyro`, `Cyclone`, which not only don't have to follow the same scheduling decisions, but are recognised names. 
> > 
> > How many people know which A57 revision will go into their servers/mobile phones? How many companies will openly publicise that kind of information?
> > 
> > So, please, remove that processor model and its associated features and let's make this about vanilla A57. This will mean test and benchmark in many variants, but at least we'll get it right for the majority of people using it.
> Sure.
> Maybe it is better to keep SchedulingPredicate IsR1P0AndLaterPred only inside ARMScheduleA57.td and make it always false? No options/platforms. To keep the information about r0px/r1px scheduling differences for future use.
If this is only used by actual cores, I don't mind it staying there, as long as they have a clear purpose.


https://reviews.llvm.org/D28152