[PATCH] D12193: [mips][p5600] Added P5600 processor and initial scheduler.

[Vasileios Kalintiris via llvm-commits]

vkalintiris accepted this revision.
vkalintiris added a comment.
This revision is now accepted and ready to land.

Generally, this LGTM, as far as I can tell. I added some comments inline.

Also, we should change the itinerary used in BC16_MMR6_DESC which was introduced in r246963.

IMHO, we should group together the ProcResourses, ItinRWs etc, in a later patch, as it improves the readability of the scheduler.


================
Comment at: lib/Target/Mips/Mips.td:172
@@ +171,3 @@
+def ImplP5600 : SubtargetFeature<"p5600", "ProcImpl", "CPU::P5600",
+                                 "The P5600 Processor", [FeatureMips32r2]>;
+
----------------
Is there a reason for using FeatureMips32r2 instead of FeatureMips32r5?

================
Comment at: lib/Target/Mips/MipsScheduleP5600.td:16
@@ +15,3 @@
+
+  let CompleteModel = 1;
+}
----------------
Shouldn't we set this to zero? This is my understanding (from the relevant comment in the definition of CompleteModel) given that we don't define itineraries for every instruction.

================
Comment at: lib/Target/Mips/MipsSubtarget.h:45
@@ -44,1 +44,3 @@
 
+  enum class CPU { P5600 };
+
----------------
vkalintiris wrote:
> Can we change these names to MipsCPUEnum and MipsProcImpl, or something similar in order to follow the naming convention of MipsArchEnum and MipsArchVersion?
I didn't investigate the problem but I can compile this patch only with an unscoped enumeration.

================
Comment at: lib/Target/Mips/MipsSubtarget.h:45-53
@@ -44,5 +44,11 @@
 
+  enum class CPU { P5600 };
+
   // Mips architecture version
   MipsArchEnum MipsArchVersion;
 
+  // Processor implementation (unused but required to exist by
+  // tablegen-erated code).
+  CPU ProcImpl;
+
   // IsLittle - The target is Little Endian
----------------
Can we change these names to MipsCPUEnum and MipsProcImpl, or something similar in order to follow the naming convention of MipsArchEnum and MipsArchVersion?


http://reviews.llvm.org/D12193