This is an archive of the discontinued LLVM Phabricator instance.

Introduce codegen for the Signal Processing Engine
ClosedPublic

Authored by jhibbits on Mar 23 2018, 7:27 AM.

Details

Summary

The Signal Processing Engine (SPE) is found on NXP/Freescale e500v1,
e500v2, and several e200 cores. This adds support targeting the e500v2,
as this is more common than the e500v1, and is in SoCs still on the
market.

This patch is very intrusive because the SPE is binary incompatible with
the traditional FPU. After discussing with others, the cleanest
solution was to make both SPE and FPU features on top of a base PowerPC
subset, so all FPU instructions are now wrapped with HasFPU predicates.

Supported by this are:

  • Code generation following the SPE ABI at the LLVM IR level (calling

conventions)

  • Single- and Double-precision math at the level supported by the APU.

Still to do:

  • Vector operations
  • SPE intrinsics

Diff Detail

Repository
rL LLVM

Event Timeline

jhibbits created this revision.Mar 23 2018, 7:27 AM

This is much easier to follow now that it is much smaller. Thank you for breaking it apart.

Also, can you please add test cases for the invalid combinations of target features (both implicit and explicit)?
Things like -mcpu=pwr8 -mattr=+spe, etc.

lib/Target/PowerPC/PPCAsmPrinter.cpp
513 ↗(On Diff #139595)

I think it would be appropriate to do this check only in debug builds, so I'd wrap this into
#ifndef NDEBUG.

518 ↗(On Diff #139595)

s/reg/Reg

520 ↗(On Diff #139595)

Don't you have to check for classes like VSSRCRegClass, VSFRCRegClass, etc. I imagine VSX scalar instructions can't exist with SPE ones either, right? Can SPE even coexist with Altivec/VSX vectors?

lib/Target/PowerPC/PPCCallingConv.td
182 ↗(On Diff #139595)

Line too long.

249 ↗(On Diff #139595)

I would probably use "common" instead of "base" (and perhaps CSR_SVR432_COMM), but I don't feel all that strong about it.

lib/Target/PowerPC/PPCFastISel.cpp
1997 ↗(On Diff #139595)

Can you please use the same variable name to be consistent? You have uses of UseSPE as well as HasSPE in this code. The latter seems to be more in-line with similar predicates.

lib/Target/PowerPC/PPCFrameLowering.cpp
1914 ↗(On Diff #139595)

This entire block is common with the one directly above it. Can you combine them and just use HasSPESaveArea in a conditional operator on the only line that is different? If you do that, please update the comment to specify that the two save areas have the same alignment requirements.

lib/Target/PowerPC/PPCInstrInfo.cpp
275 ↗(On Diff #139595)

We've recently completely changed the handling for these functions for spills and restores, so this part will need to be reworked to merge with ToT.

lib/Target/PowerPC/PPCRegisterInfo.cpp
150 ↗(On Diff #139595)

There is a note below describing the alternate allocation order of SPE registers for PPC64 SVR4 ABI. This somewhat implies that if R2 is allocatable (i.e. no TOC uses), that it needs to be saved/restored. Do we not want to make R2 a callee-saved register if it is allocatable?

lib/Target/PowerPC/PPCRegisterInfo.td
301 ↗(On Diff #139595)

I may have mixed things up here, but for some reason I was under the impression that SPE isn't available on 64-bit PPC targets.

nemanjai requested changes to this revision.Apr 9 2018, 2:55 PM

This will certainly need another cycle to at least account for the recent changes in PPCInstrInfo.cpp.

This revision now requires changes to proceed.Apr 9 2018, 2:55 PM
jhibbits marked 8 inline comments as done.Apr 10 2018, 2:54 PM
jhibbits added inline comments.
lib/Target/PowerPC/PPCInstrInfo.cpp
275 ↗(On Diff #139595)

I've recently rebased and reconciled. I'll update the review shortly.

lib/Target/PowerPC/PPCRegisterInfo.td
301 ↗(On Diff #139595)

Yeah, this was a copy&paste. Doesn't make sense to have the body.

jhibbits updated this revision to Diff 143499.Apr 22 2018, 8:06 PM
jhibbits marked an inline comment as done.

Rebase against newer head. Address several comments.

nemanjai accepted this revision.May 1 2018, 3:32 PM

Thank you so much for your patience with this long review cycle. Aside from a few minor nits, LGTM.
I also imagine you'll need something for the P9 scheduler since we've now marked the model as complete. Probably something along the lines of:

Index: lib/Target/PowerPC/PPCScheduleP9.td
===================================================================
--- lib/Target/PowerPC/PPCScheduleP9.td (revision 331260)
+++ lib/Target/PowerPC/PPCScheduleP9.td (working copy)
@@ -35,8 +35,9 @@
 
   let CompleteModel = 1;
 
-  // Do not support QPX (Quad Processing eXtension) on Power 9.
-  let UnsupportedFeatures = [HasQPX];
+  // Do not support QPX (Quad Processing eXtension) or SPE (Signal Processing
+  // Engine) on Power 9.
+  let UnsupportedFeatures = [HasQPX, HasSPE];
 
 }
lib/Target/PowerPC/PPCAsmPrinter.cpp
516 ↗(On Diff #143499)

I think this can be a range-based for loop over MI->operands().

lib/Target/PowerPC/PPCCallingConv.td
271 ↗(On Diff #143499)

Nit: full sentence comments with a period.

This revision is now accepted and ready to land.May 1 2018, 3:32 PM
jhibbits updated this revision to Diff 155610.Jul 15 2018, 8:02 PM

Update diff to fix a test with the new register order.

I don't see anything in the update that would cause me to reverse the approval, so I'd say this is still fine.

This revision was automatically updated to reflect the committed changes.