This is an archive of the discontinued LLVM Phabricator instance.

Handle subregs and superregs in callee-saved register mask
ClosedPublic

Authored by jrtc27 on Jan 22 2020, 3:43 AM.

Details

Summary

If a target lists both a subreg and a superreg in a callee-saved
register mask, the prolog will spill both aliasing registers. Instead,
don't spill the subreg if a superreg is being spilled. This case is hit by the
PowerPC SPE code, as well as a modified RISC-V backend for CHERI I maintain out
of tree.

Diff Detail

Event Timeline

jrtc27 created this revision.Jan 22 2020, 3:43 AM
bjope added a subscriber: bjope.Jan 22 2020, 5:31 AM
jhibbits accepted this revision.Jan 22 2020, 7:03 AM

Thanks for doing this! It's been on my TODO list for a little while already for SPE, but I never got around to it.

Is there any provision for spilling only the subreg, not the superreg? It seems to me that whenever the subreg needs spilled, it always also spills the superreg, even if the full superreg is not used.

This revision is now accepted and ready to land.Jan 22 2020, 7:03 AM
fhahn added a subscriber: fhahn.Jan 22 2020, 11:10 AM
fhahn added inline comments.
llvm/lib/CodeGen/PrologEpilogInserter.cpp
408

nit: I think you could use MCRegisterInfo::subregs() which is slightly less code.

arsenm added a subscriber: arsenm.Jan 22 2020, 11:13 AM
arsenm added inline comments.
llvm/lib/CodeGen/PrologEpilogInserter.cpp
408

These masks should eventually be in terms of regunits

jrtc27 marked an inline comment as done.Jan 23 2020, 2:37 AM

Thanks for doing this! It's been on my TODO list for a little while already for SPE, but I never got around to it.

Is there any provision for spilling only the subreg, not the superreg? It seems to me that whenever the subreg needs spilled, it always also spills the superreg, even if the full superreg is not used.

No, because restoring a subreg often has the side-effect of clearing the rest of the superreg, thereby violating the callee-preserved nature of them. I don't know whether that applies to SPE or not (and therefore whether the previously generated code was technically wrong), but it certainly applies to CHERI RISC-V.

llvm/lib/CodeGen/PrologEpilogInserter.cpp
408

Ah yes, I remember seeing MCRegisterInfo::subregs() go in but originally wrote this patch (not for SPE) over a year ago. I will update to the nicer API.

jrtc27 updated this revision to Diff 240021.Jan 23 2020, 2:57 PM

Switch from explicit iterator to recently-added range-based for loop interface. Ok to merge?

Ping?

Ping landing this?

jrtc27 updated this revision to Diff 362423.Jul 28 2021, 9:10 AM

Post-D101282 fp-strict.ll also has SPE check lines that get improved by this patch

It appears that this is approved but it keeps getting pinged. @jrtc27 Do you need someone to land this for you or are you hoping that someone else also approve this (perhaps @fhahn or @arsenm)?

I was speaking with @jhibbits earlier who intends to sanity check the updated SPE test changes, not that I expect there to be any issues

jhibbits accepted this revision.Jul 29 2021, 8:39 AM

The spe.ll respin looks fine to me.

This revision was landed with ongoing or failed builds.Jul 29 2021, 8:53 AM
This revision was automatically updated to reflect the committed changes.