This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Remove the SPE4RC register class and instead add f32 to the GPRC register class.
ClosedPublic

Authored by craig.topper on Sep 12 2019, 12:10 PM.

Details

Summary

Since the SPE4RC register class contains an identical set of registers
and an identical spill size to the GPRC class its slightly confusing
the tablegen emitter. It's preventing the GPRC_and_GPRC_NOR0 synthesized
register class from inheriting VTs and AltOrders from GPRC or GPRC_NOR0.
This is because SPE4C is found first in the super register class list
when inheriting these properties and it doesn't set the VTs or
AltOrders the same way as GPRC or GPRC_NOR0.

This patch replaces all uses of GPE4RC with GPRC and allows GPRC and
GPRC_NOR0 to contain f32.

The test changes here are because the AltOrders are being inherited
to GPRC_NOR0 now.

Found while trying to determine if getCommonSubClass needs to take
a VT argument. It was originally added to support fp128 on x86-64,
I've changed some things about that so that it might be needed
anymore. But a PowerPC test crashed without it and I think its
due to this subclass issue.

Diff Detail

Repository
rL LLVM

Event Timeline

craig.topper created this revision.Sep 12 2019, 12:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 12 2019, 12:10 PM

Thanks for getting rid of SPE4RC, I really did not like it, but couldn't think of a better way. Glad you did.

It looks like you're changing more than just removing SPE4RC in this diff. Do the tests really change because of the removal?

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
2457 ↗(On Diff #219967)

Is this an extra change? Doesn't look to be needed for the SPE4RC removal.

craig.topper marked an inline comment as done.Sep 12 2019, 1:23 PM

Thanks for getting rid of SPE4RC, I really did not like it, but couldn't think of a better way. Glad you did.

It looks like you're changing more than just removing SPE4RC in this diff. Do the tests really change because of the removal?

Test changes are because the GPRC_GPRC_NOR0 register class was being inferred by tablegen, but was not inheriting the AltOrder from GPRC/GPRC_NOR0. Now its inherited and this causes R2 to be at the end of the allocation order list. This caused different registers to be used and removed the spill and restore of R2.

llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
2457 ↗(On Diff #219967)

This is related to the removal of SOK_SPE4Spill which caused SOK_LastOpcodeSpill to become one less so the entry in the array needed to be removed.

jhibbits added inline comments.Sep 12 2019, 2:20 PM
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
2457 ↗(On Diff #219967)

Not entirely. The Power 9 block was not updated when SPE was added, only the Power 8 block was. Your removal on the Power 9 blocks, in both load and store cases, is unnecessary, and would likely break things. The only ones that should be removed are PPC::SPE*.

craig.topper marked an inline comment as done.Sep 12 2019, 2:31 PM
craig.topper added inline comments.
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
2457 ↗(On Diff #219967)

My mistake. I naively assumed the arrays were the same size without checking. I forgot that C++ would assume 0 for the unmentioned elements. But give a warning for the array being too large which it did for the Power8 array.

Remove bad change

jhibbits accepted this revision.Sep 12 2019, 2:42 PM
This revision is now accepted and ready to land.Sep 12 2019, 2:42 PM
This revision was automatically updated to reflect the committed changes.