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

[Vasileios Kalintiris via llvm-commits]

vkalintiris added inline comments.

================
Comment at: lib/Target/Mips/MipsSubtarget.h:45
@@ -44,1 +44,3 @@
 
+  enum class CPU { P5600 };
+
----------------
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.


http://reviews.llvm.org/D12193