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

[Daniel Sanders via llvm-commits]

dsanders marked 9 inline comments as done.

================
Comment at: lib/Target/Mips/MipsScheduleP5600.td:16
@@ +15,3 @@
+
+  let CompleteModel = 1;
+}
----------------
mpf wrote:
> vkalintiris wrote:
> > 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.
> I have some concerns about this as we are potentially making it impossible to use the P5600 scheduler (in a release version of LLVM) because someone happens to hit an instruction that did not get triggered in any testing of p5600. 99% of the scheduler would be OK in that environment with just a few corner cases of bad scheduling information.
> 
> Do other architectures set CompleteModel to '1'? If so then fine otherwise can you make a production/release build of the compiler just generate a warning if it hits a missing instruction (but continue) and have a debug compiler abort (or something along the same lines)? Just trying to make sure we don't end up with this scheduler being in place but unusable because of a corner case. Bearing in mind that if one file in an application can't build with the p5600 scheduler then many build systems will need to turn it off for all files.
21 out of 25 currently have CompleteModel = 1.

I just looked up the code for the error and there's an '#ifndef NDEBUG' around the check so release builds will invent some default scheduling information and carry on.

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


http://reviews.llvm.org/D12193