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

[Daniel Sanders via llvm-commits]

dsanders added inline comments.

================
Comment at: lib/Target/Mips/Mips.td:172
@@ +171,3 @@
+def ImplP5600 : SubtargetFeature<"p5600", "ProcImpl", "CPU::P5600",
+                                 "The P5600 Processor", [FeatureMips32r2]>;
+
----------------
vkalintiris wrote:
> Is there a reason for using FeatureMips32r2 instead of FeatureMips32r5?
I'll fix that. When this patch was first written (around a year ago) FeatureMips32r5 didn't exist.

================
Comment at: lib/Target/Mips/MipsScheduleP5600.td:16
@@ +15,3 @@
+
+  let CompleteModel = 1;
+}
----------------
vkalintiris wrote:
> 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.
We should cover every instruction present in P5600 at this point. If I've missed any then that's a bug in the scheduler and it should be fixed. If CompleteModel is 1, we will assert on unexpected instructions but if it's 0 then we will silently assign some scheduling information.

================
Comment at: lib/Target/Mips/MipsSubtarget.h:45
@@ -44,1 +44,3 @@
 
+  enum class CPU { P5600 };
+
----------------
vkalintiris wrote:
> 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.
Could you tell me which compiler and version you are using and the error message you get?

================
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
----------------
dsanders wrote:
> vkalintiris wrote:
> > 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.
> Could you tell me which compiler and version you are using and the error message you get?
I'd actually like to change MipsArchEnum and MipsArchVersion at some point. They're inside a class called MipsSubtarget so the repeated 'Mips' is redundant.


http://reviews.llvm.org/D12193