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

[Vasileios Kalintiris via llvm-commits]

vkalintiris added inline comments.

================
Comment at: lib/Target/Mips/MipsScheduleP5600.td:16
@@ +15,3 @@
+
+  let CompleteModel = 1;
+}
----------------
dsanders wrote:
> 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.
> If I've missed any then that's a bug in the scheduler and it should be fixed.

Okay then that sounds fine.

================
Comment at: lib/Target/Mips/MipsSubtarget.h:45
@@ -44,1 +44,3 @@
 
+  enum class CPU { P5600 };
+
----------------
dsanders wrote:
> 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.
GCC 4.8.4, here's the error:

`In file included from /home/vk/repos/llvm/lib/Target/Mips/MipsSubtarget.cpp:32:0:
lib/Target/Mips/MipsGenSubtargetInfo.inc: In member function ‘void llvm::MipsSubtarget::ParseSubtargetFeatures(llvm::StringRef, llvm::StringRef)’:
lib/Target/Mips/MipsGenSubtargetInfo.inc:760:43: error: ‘CPU’ is not a class, namespace, or enumeration
   if (Bits[Mips::ImplP5600] && ProcImpl < CPU::P5600) ProcImpl = CPU::P5600;
                                           ^
lib/Target/Mips/MipsGenSubtargetInfo.inc:760:66: error: ‘CPU’ is not a class, namespace, or enumeration
   if (Bits[Mips::ImplP5600] && ProcImpl < CPU::P5600) ProcImpl = CPU::P5600;
                                                                  ^
[99/461] Building CXX object tools/clang/lib/Sema/CMakeFiles/clangSema.dir/SemaTemplate.cpp.o
ninja: build stopped: subcommand failed.`

And here's the quick fix I had in my branch:
`
diff --git a/lib/Target/Mips/Mips.td b/lib/Target/Mips/Mips.td
index 87e013d..ef28ebf 100644
--- a/lib/Target/Mips/Mips.td
+++ b/lib/Target/Mips/Mips.td
@@ -175,7 +175,7 @@ def FeatureUseTCCInDIV : SubtargetFeature<
 // Mips processors supported.
 //===----------------------------------------------------------------------===//
%%%
-def ImplP5600 : SubtargetFeature<"p5600", "ProcImpl", "CPU::P5600",
+def ImplP5600 : SubtargetFeature<"p5600", "ProcImpl", "P5600",
                                  "The P5600 Processor", [FeatureMips32r2]>;
 
 class Proc<string Name, list<SubtargetFeature> Features>
diff --git a/lib/Target/Mips/MipsSubtarget.h b/lib/Target/Mips/MipsSubtarget.h
index 19e9788..fa6225c 100644
--- a/lib/Target/Mips/MipsSubtarget.h
+++ b/lib/Target/Mips/MipsSubtarget.h
@@ -42,14 +42,15 @@ class MipsSubtarget : public MipsGenSubtargetInfo {
     Mips3, Mips4, Mips5, Mips64, Mips64r2, Mips64r3, Mips64r5, Mips64r6
   };
 
-  enum class CPU { P5600 };
-
   // Mips architecture version
   MipsArchEnum MipsArchVersion;
 
+  enum CPU { P5600 };
+
+  CPU ProcImpl;
+
   // Processor implementation (unused but required to exist by
   // tablegen-erated code).
-  CPU ProcImpl;
 
   // IsLittle - The target is Little Endian
   bool IsLittle;`

================
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
----------------
vkalintiris wrote:
> dsanders wrote:
> > 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.
> GCC 4.8.4, here's the error:
> 
> `In file included from /home/vk/repos/llvm/lib/Target/Mips/MipsSubtarget.cpp:32:0:
> lib/Target/Mips/MipsGenSubtargetInfo.inc: In member function ‘void llvm::MipsSubtarget::ParseSubtargetFeatures(llvm::StringRef, llvm::StringRef)’:
> lib/Target/Mips/MipsGenSubtargetInfo.inc:760:43: error: ‘CPU’ is not a class, namespace, or enumeration
>    if (Bits[Mips::ImplP5600] && ProcImpl < CPU::P5600) ProcImpl = CPU::P5600;
>                                            ^
> lib/Target/Mips/MipsGenSubtargetInfo.inc:760:66: error: ‘CPU’ is not a class, namespace, or enumeration
>    if (Bits[Mips::ImplP5600] && ProcImpl < CPU::P5600) ProcImpl = CPU::P5600;
>                                                                   ^
> [99/461] Building CXX object tools/clang/lib/Sema/CMakeFiles/clangSema.dir/SemaTemplate.cpp.o
> ninja: build stopped: subcommand failed.`
> 
> And here's the quick fix I had in my branch:
> `
> diff --git a/lib/Target/Mips/Mips.td b/lib/Target/Mips/Mips.td
> index 87e013d..ef28ebf 100644
> --- a/lib/Target/Mips/Mips.td
> +++ b/lib/Target/Mips/Mips.td
> @@ -175,7 +175,7 @@ def FeatureUseTCCInDIV : SubtargetFeature<
>  // Mips processors supported.
>  //===----------------------------------------------------------------------===//
> %%%
> -def ImplP5600 : SubtargetFeature<"p5600", "ProcImpl", "CPU::P5600",
> +def ImplP5600 : SubtargetFeature<"p5600", "ProcImpl", "P5600",
>                                   "The P5600 Processor", [FeatureMips32r2]>;
>  
>  class Proc<string Name, list<SubtargetFeature> Features>
> diff --git a/lib/Target/Mips/MipsSubtarget.h b/lib/Target/Mips/MipsSubtarget.h
> index 19e9788..fa6225c 100644
> --- a/lib/Target/Mips/MipsSubtarget.h
> +++ b/lib/Target/Mips/MipsSubtarget.h
> @@ -42,14 +42,15 @@ class MipsSubtarget : public MipsGenSubtargetInfo {
>      Mips3, Mips4, Mips5, Mips64, Mips64r2, Mips64r3, Mips64r5, Mips64r6
>    };
>  
> -  enum class CPU { P5600 };
> -
>    // Mips architecture version
>    MipsArchEnum MipsArchVersion;
>  
> +  enum CPU { P5600 };
> +
> +  CPU ProcImpl;
> +
>    // Processor implementation (unused but required to exist by
>    // tablegen-erated code).
> -  CPU ProcImpl;
>  
>    // IsLittle - The target is Little Endian
>    bool IsLittle;`
Agreed. Repeating the Mips-prefix is redundant, we can fix this with a later patch.


http://reviews.llvm.org/D12193