[PATCH] D21685: [ARM] Do not test for CPUs, use SubtargetFeatures (Part 2). NFCI

[Diana Picus via llvm-commits]

rovka added a comment.

Thanks for reviewing.


================
Comment at: lib/Target/ARM/ARM.td:154
@@ +153,3 @@
+// Some processors have a non-pipelined VFP coprocessor.
+def FeatureNonPipelinedVFP : SubtargetFeature<"non-pipelined-vfp",
+                                              "NonPipelinedVFP", "true",
----------------
compnerd wrote:
> I believe the proper prefix would be `un` rather than `non`.
I took the prefix from the Cortex-A8 TRM. Although, now that you mention it, they seem to use "nonpipelined" as a single word, so I'm going to switch to that.

e.g. "The VFP coprocessor is a nonpipelined floating-point execution engine that blah".

================
Comment at: lib/Target/ARM/ARMBaseInstrInfo.cpp:3029
@@ +3028,3 @@
+                                                    unsigned NumRegs) {
+  unsigned UOps = 1 + NumRegs; // One for address computation, one for each reg.
+  switch (Opc) {
----------------
compnerd wrote:
> The `NumRegs` identifies that you are adding 1 for each of the registers, the comment tail doesn't add much.
Ok, got rid of it.

================
Comment at: lib/Target/ARM/ARMSubtarget.h:61
@@ +60,3 @@
+  /// What kind of timing do load multiple/store multiple instructions have.
+  enum ARMLdStMultipleTiming {
+    /// Can load/store 2 registers/cycle.
----------------
MatzeB wrote:
> echristo wrote:
> > This feels a little awkward, but I guess not terrible. I'm not sure I like it more than just having it all explicit in routines. Nothing binding here, just commentary of "is there something better here?"
> I share the sentiment of this looking strange. And there is something better: Using new-style scheduling models ("MCSchedule.h") instead of the itineraries ("MCInstrItineraries.h") will make those functions go away and the information comes directly out of the scheduling model. However this would mean rewriting a bunch of scheduling definitions which is out of scope for this patch.
That's good news, because some of the code around itineraries is really messy. E.g. ARMBaseInstrInfo::getOperandLatency and the static adjustDefLatency seem to contain some copy-pasted code that has grown just slightly different over time. 


http://reviews.llvm.org/D21685