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

[Daniel Sanders via llvm-commits]

dsanders marked 4 inline comments as done.

================
Comment at: lib/Target/Mips/Mips.td:172
@@ +171,3 @@
+def ImplP5600 : SubtargetFeature<"p5600", "ProcImpl", "CPU::P5600",
+                                 "The P5600 Processor", [FeatureMips32r2]>;
+
----------------
dsanders wrote:
> 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.
Fixed in the commit

================
Comment at: lib/Target/Mips/MipsSubtarget.h:45
@@ -44,1 +44,3 @@
 
+  enum class CPU { P5600 };
+
----------------
vkalintiris wrote:
> dsanders wrote:
> > vkalintiris wrote:
> > > 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.
> > Thanks. It's my understanding that we only support the last two major releases of GCC which is currently 4.9 and 5.0 so I don't think we should worry about this too much.
> > 
> > That said, does 'MipsSubtarget::CPU::P5600' work?
> Yes, it works.
Ok, I've made that change in the commit.


http://reviews.llvm.org/D12193