This is an archive of the discontinued LLVM Phabricator instance.

[mips][p5600] Added P5600 processor and initial scheduler.
ClosedPublic

Authored by dsanders on Aug 20 2015, 6:05 AM.

Details

Summary

The P5600 is an out-of-order, superscalar implementation of the MIPS32R5
architecture.

The scheduler has a few missing details (see the 'Tricky Instructions'
section and some quirks of the P5600 are deliberately omitted due to
implementation difficulty and low chance of significant benefit (e.g. the
predicate on P5600WriteEitherALU). However, testing on SingleSource is
showing significant performance benefits on some apps (seven in the 10-30%
range) and only one significant regression (12%) when
-pre-RA-sched=linearize is given. Without -pre-RA-sched=linearize the
results are more variable. Some do even better (up to 55% improvement) but
increased numbers of copies are slowing others down (up to 12%).

Overall, the scheduler as it currently stands is a 2.4% win with
-pre-RA-sched=linearize and a 2.7% win without -pre-RA-sched=linearize.
I'm sure we can improve on this further.

For completeness, the FPGA this was tested on shows some failures with and
without the P5600 scheduler. These appear to be scheduling related since
the two test runs have fairly different sets of failing tests even after
accounting for other factors (e.g. spurious connection failures) however
it's not P5600 specific since we also get some for the generic scheduler.

Depends on D12190

Diff Detail

Event Timeline

dsanders updated this revision to Diff 32689.Aug 20 2015, 6:05 AM
dsanders retitled this revision from to [mips][p5600] Added P5600 processor and initial scheduler..
dsanders updated this object.
dsanders added subscribers: vkalintiris, atrick, llvm-commits.
vkalintiris accepted this revision.Sep 16 2015, 2:43 PM
vkalintiris edited edge metadata.

Generally, this LGTM, as far as I can tell. I added some comments inline.

Also, we should change the itinerary used in BC16_MMR6_DESC which was introduced in r246963.

IMHO, we should group together the ProcResourses, ItinRWs etc, in a later patch, as it improves the readability of the scheduler.

lib/Target/Mips/Mips.td
172

Is there a reason for using FeatureMips32r2 instead of FeatureMips32r5?

lib/Target/Mips/MipsScheduleP5600.td
16

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.

lib/Target/Mips/MipsSubtarget.h
45

I didn't investigate the problem but I can compile this patch only with an unscoped enumeration.

45–53

Can we change these names to MipsCPUEnum and MipsProcImpl, or something similar in order to follow the naming convention of MipsArchEnum and MipsArchVersion?

This revision is now accepted and ready to land.Sep 16 2015, 2:43 PM
dsanders added inline comments.Sep 24 2015, 4:06 AM
lib/Target/Mips/Mips.td
172

I'll fix that. When this patch was first written (around a year ago) FeatureMips32r5 didn't exist.

lib/Target/Mips/MipsScheduleP5600.td
16

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.

lib/Target/Mips/MipsSubtarget.h
45

Could you tell me which compiler and version you are using and the error message you get?

45–53

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.

vkalintiris added inline comments.Sep 24 2015, 5:12 AM
lib/Target/Mips/MipsScheduleP5600.td
16

If I've missed any then that's a bug in the scheduler and it should be fixed.

Okay then that sounds fine.

lib/Target/Mips/MipsSubtarget.h
45

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;`

45–53

Agreed. Repeating the Mips-prefix is redundant, we can fix this with a later patch.

mpf added a subscriber: mpf.Sep 24 2015, 6:19 AM
mpf added inline comments.
lib/Target/Mips/MipsScheduleP5600.td
16

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.

dsanders marked 9 inline comments as done.Sep 24 2015, 6:39 AM
dsanders added inline comments.
lib/Target/Mips/MipsScheduleP5600.td
16

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.

lib/Target/Mips/MipsSubtarget.h
45

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?

vkalintiris added inline comments.Sep 24 2015, 7:19 AM
lib/Target/Mips/MipsSubtarget.h
45

Yes, it works.

dsanders closed this revision.Sep 28 2015, 11:25 AM
dsanders marked an inline comment as done.
dsanders marked 4 inline comments as done.Sep 28 2015, 11:26 AM
dsanders added inline comments.
lib/Target/Mips/Mips.td
172

Fixed in the commit

lib/Target/Mips/MipsSubtarget.h
45

Ok, I've made that change in the commit.